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

#639's redaction fix has no regression test — both leaks can come back green #656

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

Follow-up to #639, merged as PR #655 (now on main at 3fab743).

What is missing

PR #655 rewrote redact() in scripts/config-edit.sh so a masked key's value is masked by file line number, not by an indentation anchor. It changed no test file. scripts/test-config-edit.sh has 20 criteria, and criteria 15a/15b cover a block scalar whose key line is inside the printed hunk. Neither of the two cases #639 was actually about has a test:

  • A. the key line sits outside the 3-line diff -u context, so there is no anchor to find;
  • B. a blank line inside the value resets the anchor.

So if someone restores the old indentation-anchored redact(), the whole suite stays green and the secret leak comes back silently. This is a redaction function — the one place in the script whose job is to stop a credential reaching the terminal — so it should not be the part with no guard.

I measured both cases myself, outside the suite

Fixture A — token: | with an 8-line body, one line changed in the middle, so diff -u prints the body with the token: key line 5 lines above the hunk:

$ diff -u a_old.yaml a_new.yaml | redact          # redact() as on origin/main
@@ -6,7 +6,7 @@
     SECRET-LINE-3
     SECRET-LINE-4
     SECRET-LINE-5
-    SECRET-LINE-6
+    SECRET-LINE-6-CHANGED
     SECRET-LINE-7
     SECRET-LINE-8

Same fixture, redact() as merged from PR #655:

@@ -6,7 +6,7 @@
     <redacted>
     <redacted>
     <redacted>
-    <redacted>
+    <redacted>
     <redacted>
     <redacted>

Fixture B — a blank line inside the block scalar. On origin/main the key line printed token: <redacted>, the line above the blank printed <redacted>, and then LEAK-AFTER-BLANK / LEAK-AFTER-BLANK-CHANGED printed raw below it. After PR #655 all of them print <redacted>.

Control, in both fixtures: control: CTRL-MUST-APPEAR still printed unmasked, so the new code is not a blanket masker.

Positive control, so a clean result is not a broken fixture: the plain diff -u before redaction did contain the secret body in every case. I checked that first.

The ask

Add two criteria to scripts/test-config-edit.sh, in the style of the existing 15a/15b:

  1. key line outside the hunk — a fixture with a masked key whose block-scalar body is long enough that diff -u prints the body without the key line. Assert the secret text is absent from the output and assert a non-secret control line is present. The second half is required: an assert_not_contains alone passes when the fixture breaks and prints nothing.
  2. blank line inside the value — a fixture with a blank line inside the block scalar and a changed line after it. Same two assertions.

Each new test must be RED on origin/main's redact() and GREEN on the current one. Say in the report which command you used to show the RED, with its real output. A test that is not red against the old code is not a guard.

Out of scope: do not change redact() itself. This ticket is the missing test only.

Where the gap came from

The #639 brief asked for the fix and for RED-before evidence, and the worker gave both — the evidence was run in a throwaway Bash harness, which is enough to prove the fix and leaves nothing behind in the repo. The brief never asked for the evidence to be committed as a test. That is a defect in my brief, not in the work.

Follow-up to #639, merged as PR #655 (now on `main` at `3fab743`). ## What is missing PR #655 rewrote `redact()` in `scripts/config-edit.sh` so a masked key's value is masked by **file line number**, not by an indentation anchor. It changed no test file. `scripts/test-config-edit.sh` has 20 criteria, and criteria 15a/15b cover a block scalar **whose key line is inside the printed hunk**. Neither of the two cases #639 was actually about has a test: - **A.** the key line sits outside the 3-line `diff -u` context, so there is no anchor to find; - **B.** a blank line inside the value resets the anchor. So if someone restores the old indentation-anchored `redact()`, the whole suite stays green and the secret leak comes back silently. This is a redaction function — the one place in the script whose job is to stop a credential reaching the terminal — so it should not be the part with no guard. ## I measured both cases myself, outside the suite Fixture A — `token: |` with an 8-line body, one line changed in the middle, so `diff -u` prints the body with the `token:` key line 5 lines above the hunk: ``` $ diff -u a_old.yaml a_new.yaml | redact # redact() as on origin/main @@ -6,7 +6,7 @@ SECRET-LINE-3 SECRET-LINE-4 SECRET-LINE-5 - SECRET-LINE-6 + SECRET-LINE-6-CHANGED SECRET-LINE-7 SECRET-LINE-8 ``` Same fixture, `redact()` as merged from PR #655: ``` @@ -6,7 +6,7 @@ <redacted> <redacted> <redacted> - <redacted> + <redacted> <redacted> <redacted> ``` Fixture B — a blank line inside the block scalar. On `origin/main` the key line printed `token: <redacted>`, the line above the blank printed `<redacted>`, and then `LEAK-AFTER-BLANK` / `LEAK-AFTER-BLANK-CHANGED` printed raw below it. After PR #655 all of them print `<redacted>`. Control, in both fixtures: `control: CTRL-MUST-APPEAR` still printed unmasked, so the new code is not a blanket masker. Positive control, so a clean result is not a broken fixture: the plain `diff -u` before redaction did contain the secret body in every case. I checked that first. ## The ask Add two criteria to `scripts/test-config-edit.sh`, in the style of the existing 15a/15b: 1. **key line outside the hunk** — a fixture with a masked key whose block-scalar body is long enough that `diff -u` prints the body without the key line. Assert the secret text is absent from the output **and** assert a non-secret control line is present. The second half is required: an `assert_not_contains` alone passes when the fixture breaks and prints nothing. 2. **blank line inside the value** — a fixture with a blank line inside the block scalar and a changed line after it. Same two assertions. Each new test must be **RED on `origin/main`'s `redact()` and GREEN on the current one**. Say in the report which command you used to show the RED, with its real output. A test that is not red against the old code is not a guard. Out of scope: do not change `redact()` itself. This ticket is the missing test only. ## Where the gap came from The #639 brief asked for the fix and for RED-before evidence, and the worker gave both — the evidence was run in a throwaway Bash harness, which is enough to prove the fix and leaves nothing behind in the repo. The brief never asked for the evidence to be **committed as a test**. That is a defect in my brief, not in the work.
ltms commented 2026-10-03 16:11:34 +02:00
Author
Owner
Copy Link
Copy Source

Merged locally as PR #658, on main at 905fa3a. PR closed by hand. Both leaks now have a committed guard.

I verified the RED and the GREEN myself rather than taking the report, because the RED is the whole finding here:

  • Criterion 19 — with the current redact(): suite exit 0, PASS. With git show 28ea0de:scripts/config-edit.sh swapped in: exit 1, FAIL: … must NOT contain [SECRET-LINE-6-CHANGED], but it does.
  • Criterion 20 — set -e aborts the suite at 19, so 20 never gets a turn in that pass. I skipped 19's invocation only and re-ran against the old redactor: exit 1, FAIL: … must NOT contain [LEAK-AFTER-BLANK-CHANGED], but it does. Then the control: the same skip with the current redactor gives exit 0 and PASS — so the pass is the fix, not the skip.

Both tests restored cleanly: git status --porcelain empty after each probe.

The structural detail that makes these real guards: in both tests the assert_contains positive control runs before the assert_not_contains. A broken fixture therefore fails loudly on the missing control line instead of sailing through the absence check. That ordering was the point of the ask and the worker got it right.

The worker also handled the set -e problem honestly — it noticed 20 could not run, isolated it with a temporary wrapper, said so, and deleted the wrapper. It would have been easy to report "both RED" from a single run that only ever exercised one.

Correction to an earlier version of this comment. I first wrote that the worker's flag about acceptance criterion 11 ("a backup is never committable") would be filed as a follow-up ticket. I then checked criterion 11 and it is sound — there is no follow-up to file, and no #660.

It does have a positive control, two lines before the assertions the worker was worried about:

backup="$(ls -t "$dir"/.config-backups/fleetd.yaml.bak.* 2>/dev/null | head -1)"
[ -n "$backup" ] || fail "no backup found under .config-backups/ — did the location change?"

So if no backup is produced, the test fails there rather than passing the gitignore checks vacuously. I also checked the thing that would have made git check-ignore meaningless: TMP is created inside the repo (TMP="$(mktemp -d "$ROOT/.config-edit-test.XXXXXX")", line 14), so the backup really is a repo-relative path and the ignore check is a real check rather than a no-op on an outside path.

The worker said plainly that it had not looked closely enough to be sure and changed nothing. That was the right call, and reporting a suspicion it could not confirm is better than staying quiet — it just happened to be a false positive, and the cost of checking was two commands.

Merged locally as PR #658, on `main` at `905fa3a`. PR closed by hand. Both leaks now have a committed guard. **I verified the RED and the GREEN myself** rather than taking the report, because the RED is the whole finding here: - **Criterion 19** — with the current `redact()`: suite exit 0, `PASS`. With `git show 28ea0de:scripts/config-edit.sh` swapped in: exit 1, `FAIL: … must NOT contain [SECRET-LINE-6-CHANGED], but it does`. - **Criterion 20** — `set -e` aborts the suite at 19, so 20 never gets a turn in that pass. I skipped 19's invocation only and re-ran against the old redactor: exit 1, `FAIL: … must NOT contain [LEAK-AFTER-BLANK-CHANGED], but it does`. **Then the control:** the same skip with the *current* redactor gives exit 0 and `PASS` — so the pass is the fix, not the skip. Both tests restored cleanly: `git status --porcelain` empty after each probe. **The structural detail that makes these real guards:** in both tests the `assert_contains` positive control runs *before* the `assert_not_contains`. A broken fixture therefore fails loudly on the missing control line instead of sailing through the absence check. That ordering was the point of the ask and the worker got it right. The worker also handled the `set -e` problem honestly — it noticed 20 could not run, isolated it with a temporary wrapper, said so, and deleted the wrapper. It would have been easy to report "both RED" from a single run that only ever exercised one. --- **Correction to an earlier version of this comment.** I first wrote that the worker's flag about acceptance criterion 11 ("a backup is never committable") would be filed as a follow-up ticket. I then checked criterion 11 and **it is sound — there is no follow-up to file, and no #660.** It does have a positive control, two lines before the assertions the worker was worried about: ```bash backup="$(ls -t "$dir"/.config-backups/fleetd.yaml.bak.* 2>/dev/null | head -1)" [ -n "$backup" ] || fail "no backup found under .config-backups/ — did the location change?" ``` So if no backup is produced, the test fails there rather than passing the gitignore checks vacuously. I also checked the thing that would have made `git check-ignore` meaningless: `TMP` is created **inside** the repo (`TMP="$(mktemp -d "$ROOT/.config-edit-test.XXXXXX")"`, line 14), so the backup really is a repo-relative path and the ignore check is a real check rather than a no-op on an outside path. The worker said plainly that it had not looked closely enough to be sure and changed nothing. That was the right call, and reporting a suspicion it could not confirm is better than staying quiet — it just happened to be a false positive, and the cost of checking was two commands.
ltms closed this issue 2026-10-03 16:12:13 +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#656
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?