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.
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).
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.
Criterion 19 covers a block-scalar body whose key line falls outside
diff -u's default 3-line context (an 8-line body with only the 6th
line changed). Criterion 20 covers a blank line inside the value,
which used to reset the old indentation-anchored mask.
Both are RED against the pre-#639 redact() (git show 28ea0de) and
GREEN against the current one; each asserts both the secret's
absence and a non-secret control line's presence.
Defect 7 (comment 17670): redact() only masked a line that itself started with a
secret-looking key, so a YAML block scalar's value leaked on the lines that
followed the key while the key line right above it printed a reassuring
"<redacted>". Fixed by tracking the masked key's own indentation and masking
every following line indented deeper than it, stopping once indentation returns
to the key's level or shallower; the diff's leading +/-/space marker is stripped
before indentation is measured, per the comment's own pitfall. "passphrase" is
now also in the key-name backstop.
Defect 8 (comment 17673): apply_set_pairs echoed the operator's full
"path=value" input, unredacted, in both of its yq-failure die messages — a
failing --set with a secret-looking value printed that value right back. Fixed
to print only the path; deliberately not routed through redact, which would
pass a non-"key: value"-shaped string straight through.
Adds acceptance criteria 15a (block-scalar continuation), 15b (passphrase key),
and 16 (failing --set never echoes its value) to scripts/test-config-edit.sh,
each with a positive control proving the relevant line really was in the
printed output before asserting the secret is absent. All three confirmed RED
against the pre-fix code and GREEN after, in isolation, before being folded
into the full suite (16 criteria + 3 extras, exit 0).
Also updates PR #636's description per comment 17671: the redact() sentence now
names the continuation-masking rule and says plainly that the key-name list is
a backstop, never a complete list.
The --restore "no backup found" message still printed the old beside-the-config glob
(${CONFIG}.bak.*) even though newest_backup had already moved to searching the managed
.config-backups/ directory. The message was left behind when the search moved — the search
itself was already correct (ticket comment 17664). Fix is reporting-only: the message now
names the directory actually searched (via backup_dir_for), and separately says that a
backup written the old way, directly beside the config, is not searched any more, with the
one-line cp to recover one by hand. No search fallback was added — reading backups from
outside the managed directory stays unsupported, as instructed.
Acceptance criterion 14 proves both directions: the not-found message names the real
directory (confirmed red on the pre-fix code, green after), and a restore with a real backup
present in .config-backups/ still succeeds (confirmed this catches an "always not-found"
regression that direction 1 alone would miss).
All 14 criteria plus 3 extras pass in scripts/test-config-edit.sh.
Five fixes against PR #636, all verified by the lead's own review and reproduced here:
1. --set .a.b= (a forgotten value) is now refused outright instead of silently nulling the
field — a null numeric config value falls back to its default rather than erroring, which
widens capacity silently instead of failing loudly. A deliberate clear gets its own spelling,
--set .a.b=null, which writes a literal YAML null via yq, never through strenv(). (criteria
9, 10)
2. Backups move from beside fleetd.yaml to a dedicated fleetd/.config-backups/ directory,
gitignored at the repo root (so it also covers scripts/test-config-edit.sh's own throwaway
fixtures) and in fleetd/.gitignore, plus a fleetd.yaml.bak.* glob backstop for any stray
backup written the old way. A backup of a file that must never be committed inherits that
requirement. (criterion 11)
3. The live config's file mode now survives both an edit and a restore. mv from a mktemp
candidate used to carry mktemp's 0600 onto the live path forever, and cp onto an existing
file keeps the destination's mode, so a restore did not undo it either. (criterion 12)
4. A global CAND + single EXIT/INT/TERM trap prevents an uninstalled .config-edit.XXXXXX
candidate from leaking if the script is interrupted mid-run. No acceptance criterion is
gated on this — a reproducible leak could not be made to happen on demand — but it is cheap
and obviously right.
5. Acceptance criterion 7's redaction check gained a positive control: it now asserts the
output actually CONTAINS the redaction marker and the changed key, not only that it lacks
the secret. The prior two assertions were negative-only and passed just as happily when the
diff was never printed at all — confirmed by reproducing the lead's own mutation (deleting
the redacted diff print on the edit path) and watching it survive the old test and get
caught by the new one. (criterion 13)
All 13 acceptance criteria plus 3 extras pass in scripts/test-config-edit.sh. Criteria 9, 10,
11, 12 and 13 were each proven non-vacuous: criteria 9/10 by mutating the test's own expected
value and watching it fail by name, then reverting; criteria 11/12/13 by reverting or mutating
the corresponding fix in config-edit.sh and watching the matching criterion fail by name, then
restoring the fix and re-confirming a clean pass.
Backs up, builds a candidate off the live file, parse-checks it with yq before
install, installs atomically, then reads the daemon's own ConfigRef reload
verdict back out of fleetd.out (marked from before the edit, so a stale line
can never be mistaken for this edit's result). Four exit codes: 0 clean, 3
needs a restart, 4 refused (backup restored), 5 cannot tell (nothing
restored, printed --restore command). Every diff is redacted.
scripts/test-config-edit.sh drives it end to end against fixtures in a
throwaway temp dir, with no daemon involved.
Dai Ha