From 0db6d31dc291bb939b3ef735146988ec1e7654b0 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 1 Oct 2026 17:29:04 +0200 Subject: [PATCH 1/4] fleetd #635: add scripts/config-edit.sh, the one auditable way to edit fleetd.yaml 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. --- scripts/config-edit.sh | 569 ++++++++++++++++++++++++++++++++++++ scripts/test-config-edit.sh | 285 ++++++++++++++++++ 2 files changed, 854 insertions(+) create mode 100755 scripts/config-edit.sh create mode 100755 scripts/test-config-edit.sh diff --git a/scripts/config-edit.sh b/scripts/config-edit.sh new file mode 100755 index 0000000..2c9ef22 --- /dev/null +++ b/scripts/config-edit.sh @@ -0,0 +1,569 @@ +#!/usr/bin/env bash +# +# The one auditable way to edit the live fleetd.yaml. +# +# fleetd ticket #635 — why this exists at all: fleetd.yaml is gitignored and holds the live +# fleet's settings. A bad raw edit reaches a daemon that is already serving, so a direct `Edit` +# on it is refused by policy. This script is the allow-listed alternative, and it is not just +# convenience — it is the thing a raw file write can never give you: a backup, a parse check +# BEFORE the file is installed, and the daemon's own reload verdict read back afterwards. An +# edit to a live config is not finished when the bytes are written. It is finished when the +# daemon has said what it did with them. +# +# What the daemon says, and how this script finds it — measured against `ConfigRef.java` on +# fleetd commit 158a2a8, 2026-10-01: +# +# 1. `ConfigRef` re-reads fleetd.yaml only when the WATCHER sees the mtime move (every 10s by +# default — read the real interval out of the daemon's own startup line, "config watch: ... +# re-read when it changes (every Ns)"). So a verdict never appears before the next tick. +# 2. `ConfigRef.Outcome.summary()` logs exactly one of five strings (ConfigRef.java:371-391): +# config reload refused — +# config reload refused — these keys cannot change under a running daemon: . ... +# config reloaded +# config reloaded; these changes need a restart to take effect: +# config reloaded; partially live — +# A parse/validation failure logs a DIFFERENT line instead, before any summary ever runs +# (ConfigRef.java:425): "config reload from refused, keeping the running config: +# ". This script recognises both shapes of refusal. +# 3. The em dash in those strings is a real multi-byte character — match the stable prefix +# "config reload refused" (or "...refused, keeping the running config" for the parse-failure +# shape), never the dash itself. +# 4. A cold-key change (bind/herdrSocket/memberHerdrSocket/broker/auth) throws away the WHOLE +# reload — the running config keeps every old value, not only the cold one. +# 5. A deferred/split change IS applied (current.set(fresh) runs) — "needs a restart" is a +# SUCCESS with a follow-up, never a failure. +# +# Four outcomes, and they stay four (see the exit code table below). The one most likely to be +# gotten wrong is "cannot tell" (exit 5): the daemon may be down, or the watcher may be stalled, +# and folding that into either "refused" or "applied" is worse than never checking at all, +# because a caller then acts on a verdict nobody actually read. So exit 5 never restores — a +# visible, recoverable edit beats an invisible revert of a GOOD edit. +# +# Usage: +# scripts/config-edit.sh --check +# scripts/config-edit.sh --set = [--set ...] +# scripts/config-edit.sh --from +# scripts/config-edit.sh --dry-run --set = +# scripts/config-edit.sh --restore +# +# Overrides (so this is drivable with no daemon — see scripts/test-config-edit.sh): +# --config default: fleetd/fleetd.yaml +# --log default: fleetd/fleetd.out +# --wait-seconds default: 4x the watch interval this script reads out of --log (10 -> 40) +# +# Exit codes (the --check/--restore/usage-error paths are reported separately, see below): +# 0 applied; verdict read; clean +# 3 applied; verdict read; needs a restart (deferred or split keys named) +# 4 REFUSED by the daemon; backup restored (and the restore's own verdict reported if seen) +# 5 CANNOT TELL — no verdict line inside the wait window. Nothing is restored. +# +# Never prints a secret. fleetd.yaml keeps credentials out by indirection (broker.uriEnv, +# gitTokenEnv) but this script does not rely on that staying true: every diff it prints is piped +# through `redact`, which (a) blanks the userinfo of any `scheme://user:pass@host` and (b) masks +# the whole value on any line whose key looks like a credential. See `redact` below. + +set -euo pipefail + +REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +SELF="$REPO/scripts/config-edit.sh" + +CONFIG="$REPO/fleetd/fleetd.yaml" +LOG="$REPO/fleetd/fleetd.out" +WAIT_SECONDS_OVERRIDE="" +FALLBACK_PORT=8765 + +MODE="" +DRY_RUN=0 +SETS=() +FROM_FILE="" + +say() { printf '\n\033[1m== %s\033[0m\n' "$*"; } +ok() { printf ' ok %s\n' "$*"; } +warn() { printf ' WARN %s\n' "$*"; } +die() { printf '\n FAIL %s\n\n' "$*" >&2; exit 1; } + +set_mode() { + local new="$1" + if [ -n "$MODE" ] && [ "$MODE" != "$new" ]; then + die "cannot combine --$MODE and --$new in one invocation" + fi + MODE="$new" +} + +while [ $# -gt 0 ]; do + case "$1" in + --check) set_mode check; shift ;; + --restore) set_mode restore; shift ;; + --set) + [ $# -ge 2 ] || die "--set requires =" + set_mode set + SETS+=("$2") + shift 2 ;; + --from) + [ $# -ge 2 ] || die "--from requires a candidate file path" + set_mode from + FROM_FILE="$2" + shift 2 ;; + --dry-run) DRY_RUN=1; shift ;; + --config) + [ $# -ge 2 ] || die "--config requires a path" + CONFIG="$2"; shift 2 ;; + --log) + [ $# -ge 2 ] || die "--log requires a path" + LOG="$2"; shift 2 ;; + --wait-seconds) + [ $# -ge 2 ] || die "--wait-seconds requires a number of seconds" + WAIT_SECONDS_OVERRIDE="$2"; shift 2 ;; + -h|--help) sed -n '3,63p' "$SELF"; exit 0 ;; + *) echo "unknown option: $1 (try --help)" >&2; exit 2 ;; + esac +done + +[ -n "$MODE" ] || die "no action given — use --check, --set, --from, or --restore (see --help)" + +# ------------------------------------------------------------------------------------- redaction +# +# Two independent passes, applied to every diff this script ever prints: +# 1. `scheme://user:pass@host` -> `scheme://@host`, globally (the `g` flag matters — +# a line can carry more than one URI). +# 2. Any line whose key looks like TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY, matched +# case-insensitively against the key text (uriEnv, gitTokenEnv, ... are camelCase, not +# SCREAMING_CASE) has its whole value blanked, diff marker and indentation kept so the shape +# of the change is still visible. Deliberately conservative: a false-positive redaction on an +# unrelated line costs nothing, an unredacted secret is a security defect (acceptance +# criterion 7). +redact() { + local line marker saved_nocasematch=0 + shopt -q nocasematch && saved_nocasematch=1 + shopt -s nocasematch + sed -E 's#://[^@]*@#://@#g' | while IFS= read -r line || [ -n "$line" ]; do + if [[ "$line" =~ ^([-+\ ]?[[:space:]]*[A-Za-z0-9_.-]+:) ]]; then + marker="${BASH_REMATCH[1]}" + if [[ "$marker" =~ (TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY) ]]; then + printf '%s \n' "$marker" + continue + fi + fi + printf '%s\n' "$line" + done + [ "$saved_nocasematch" = 1 ] || shopt -u nocasematch +} + +# ------------------------------------------------------------------------------------- the probe +# +# Probe the SOCKET, never `pgrep`/`ps -f` — both print argv, and argv holds `NAME=value`, making +# either a credential channel. The port comes from the config's own `bind.port`; 8765 is only a +# fallback when that key is absent or the file does not parse yet. +resolve_port() { + local file="$1" port + if [ -f "$file" ] && command -v yq >/dev/null 2>&1; then + port="$(yq eval '.bind.port' "$file" 2>/dev/null || true)" + else + port="" + fi + case "$port" in + ''|null) echo "$FALLBACK_PORT" ;; + *) echo "$port" ;; + esac +} + +daemon_listening() { + local port="$1" + if command -v nc >/dev/null 2>&1; then + nc -z -w1 127.0.0.1 "$port" 2>/dev/null + else + ( exec 3<>"/dev/tcp/127.0.0.1/$port" ) 2>/dev/null + fi +} + +# --------------------------------------------------------------------------------- the log marker +# +# Take the log's line count BEFORE touching anything. Every later read of "what did the daemon +# say" starts strictly after this mark, so a refusal from hours ago can never be mistaken for +# this edit's verdict. Same approach as scripts/redeploy-fleetd.sh's RESTART_MARK. +log_mark() { + local file="$1" + if [ -f "$file" ]; then + wc -l < "$file" 2>/dev/null || echo 0 + else + echo 0 + fi +} + +read_verdict_after_marker() { + local file="$1" mark="$2" + [ -f "$file" ] || return 0 + tail -n "+$((mark + 1))" "$file" 2>/dev/null || true +} + +# Classifies one log LINE. Echoes one of: refused | clean | needs-restart | none. Always +# succeeds (every branch ends in `echo`), so it is safe to call from inside `$( )`. +classify_verdict_line() { + local line="$1" + case "$line" in + *'config reload refused'*) echo refused ;; + *'config reload from '*'refused, keeping the running config'*) echo refused ;; + *'config reloaded'*) + case "$line" in + *'need a restart'*|*'partially live'*) echo needs-restart ;; + *) echo clean ;; + esac ;; + *) echo none ;; + esac + return 0 +} + +scan_region_for_verdict() { + local region="$1" line kind + [ -n "$region" ] || return 1 + while IFS= read -r line || [ -n "$line" ]; do + kind="$(classify_verdict_line "$line")" + if [ "$kind" != "none" ]; then + VERDICT_KIND="$kind" + VERDICT_LINE="$line" + return 0 + fi + done <<< "$region" + return 1 +} + +# Sets VERDICT_KIND/VERDICT_LINE and returns 0 on the first verdict line found after $mark; +# returns 1 (VERDICT_KIND=none) if none appeared inside $wait_s seconds. Checks once before each +# sleep AND once more after the last sleep, the same boundary idiom +# scripts/redeploy-fleetd.sh's wait_for_daemon_exit/wait_for_new_pid already use. +wait_for_verdict() { + local log="$1" mark="$2" wait_s="$3" _i region + VERDICT_KIND="none" + VERDICT_LINE="" + for _i in $(seq "$wait_s"); do + region="$(read_verdict_after_marker "$log" "$mark")" + scan_region_for_verdict "$region" && return 0 + sleep 1 + done + region="$(read_verdict_after_marker "$log" "$mark")" + scan_region_for_verdict "$region" && return 0 + return 1 +} + +last_verdict_line() { + local file="$1" line out="" + [ -f "$file" ] || return 0 + while IFS= read -r line || [ -n "$line" ]; do + if [ "$(classify_verdict_line "$line")" != "none" ]; then + out="$line" + fi + done < "$file" + printf '%s' "$out" +} + +default_wait_seconds() { + local log="$1" interval="" + if [ -f "$log" ]; then + interval="$(grep -F 'config watch:' "$log" 2>/dev/null | tail -1 \ + | sed -E 's/.*\(every ([0-9]+)s\).*/\1/' || true)" + fi + case "$interval" in + ''|*[!0-9]*) interval=10 ;; + esac + echo $((interval * 4)) +} + +# ----------------------------------------------------------------------------------- the backup +# +# Timestamped, never pruned — "keep backups" per the ticket. A pid suffix avoids a same-second +# collision between two invocations. +backup_config() { + local src="$1" ts backup + ts="$(date -u +%Y%m%dT%H%M%S)Z" + backup="${src}.bak.${ts}.$$" + cp "$src" "$backup" \ + || die "could not create a backup at $backup — refusing to edit without one. The live config at $src was NOT touched." + printf '%s' "$backup" +} + +newest_backup() { + local cfg="$1" + ls -t "${cfg}".bak.* 2>/dev/null | head -1 || true +} + +# --------------------------------------------------------------------------- candidate builders +# +# Never edit the live file in place. Each builder fills $1 (a temp file already sitting in the +# SAME directory as the live config, so the later `mv` install is a rename, not a cross-device +# copy — see run_edit). +apply_set_pairs() { + local cand="$1" kv path value + shift + for kv in "$@"; do + case "$kv" in + *=*) : ;; + *) die "--set expects =, got: '$kv'" ;; + esac + path="${kv%%=*}" + path="${path#.}" + value="${kv#*=}" + CONFIG_EDIT_SET_VALUE="$value" yq eval -i ".${path} = strenv(CONFIG_EDIT_SET_VALUE)" "$cand" \ + || die "yq could not apply --set '$kv' — nothing was installed. The live config is unchanged." + done +} + +build_from_set() { + local cand="$1" + cp "$CONFIG" "$cand" + apply_set_pairs "$cand" "${SETS[@]}" +} + +build_from_file() { + local cand="$1" + [ -f "$FROM_FILE" ] || die "--from file not found: $FROM_FILE" + cp "$FROM_FILE" "$cand" +} + +parse_check() { + yq eval '.' "$1" >/dev/null 2>&1 +} + +install_candidate() { + local cand="$1" live="$2" + mv -f "$cand" "$live" +} + +# -------------------------------------------------------------------------------- the report path +# +# Prints the literal command the operator (or a test) can run to restore the backup by hand — the +# absolute path to THIS script plus the overrides actually in force, so it works from any cwd. +restore_command_line() { + printf '%q --restore --config %q --log %q --wait-seconds %q' "$SELF" "$CONFIG" "$LOG" "$WAIT_SECONDS" +} + +# State 4 only: restore the pre-edit backup, then wait for a SECOND verdict confirming the +# restore itself reloaded cleanly. Never claims a restore it did not observe — if the second wait +# also times out, it says so plainly rather than reporting "restored" as though confirmed. +restore_and_confirm() { + local backup="$1" mark2 + mark2="$(log_mark "$LOG")" + cp "$backup" "$CONFIG" \ + || die "could not restore $backup onto $CONFIG — the live config is left as the REFUSED edit. Fix this by hand immediately: cp \"$backup\" \"$CONFIG\"" + ok "restored from $backup" + if wait_for_verdict "$LOG" "$mark2" "$WAIT_SECONDS"; then + case "$VERDICT_KIND" in + refused) warn "the RESTORE was also refused by the daemon: $VERDICT_LINE" ;; + *) ok "restore confirmed: $VERDICT_LINE" ;; + esac + else + warn "the restore is on disk, but no confirming verdict line appeared within ${WAIT_SECONDS}s" + warn "cannot confirm the restore reloaded cleanly — check $LOG by hand" + fi + return 0 +} + +# The four-outcome decision. Echoed as a function so run_edit/restore_mode share one place that +# can return 0/3/4/5 — never duplicated, never re-worded between the two callers. +report_outcome() { + local mark="$1" backup="$2" kind line + + say "waiting for the daemon's verdict (up to ${WAIT_SECONDS}s)" + if wait_for_verdict "$LOG" "$mark" "$WAIT_SECONDS"; then + kind="$VERDICT_KIND"; line="$VERDICT_LINE" + else + kind="none" + fi + + case "$kind" in + clean) + ok "daemon verdict: $line" + say "result: applied cleanly" + return 0 ;; + needs-restart) + ok "daemon verdict: $line" + say "result: applied — a restart is needed for the change(s) named above" + return 3 ;; + refused) + warn "daemon verdict: $line" + say "result: REFUSED — restoring the backup" + restore_and_confirm "$backup" + return 4 ;; + none) + warn "no verdict line appeared within ${WAIT_SECONDS}s after $LOG line $mark" + warn "CANNOT TELL whether the daemon applied this edit, refused it, or is simply down." + warn "Nothing was restored — the edit is still on disk at $CONFIG." + echo + echo " backup: $backup" + echo " to restore it by hand:" + echo " $(restore_command_line)" + return 5 ;; + esac +} + +# ------------------------------------------------------------------------------------- the modes +check_mode() { + say "config-edit --check" + if [ -f "$CONFIG" ]; then + if parse_check "$CONFIG"; then + ok "config parses: $CONFIG" + else + warn "config does NOT parse as valid YAML: $CONFIG" + fi + else + warn "no config file at $CONFIG" + fi + + local port + port="$(resolve_port "$CONFIG")" + if daemon_listening "$port"; then + ok "daemon is listening on 127.0.0.1:$port" + else + warn "no daemon detected listening on 127.0.0.1:$port" + fi + + ok "watch interval assumed: $(( $(default_wait_seconds "$LOG") / 4 ))s (derives --wait-seconds default of $(default_wait_seconds "$LOG")s)" + + local verdict + verdict="$(last_verdict_line "$LOG")" + if [ -n "$verdict" ]; then + ok "last verdict in log: $verdict" + else + warn "no reload verdict line found in $LOG" + fi + + local backup + backup="$(newest_backup "$CONFIG")" + if [ -n "$backup" ]; then + ok "newest backup: $backup" + else + warn "no backups found for $CONFIG" + fi + + if command -v yq >/dev/null 2>&1; then + ok "yq: $(yq --version 2>&1)" + else + warn "yq not found on PATH" + fi + + return 0 +} + +# Shared by --set and --from: backup, build, parse-check, redacted diff, install, await verdict. +run_edit() { + local builder="$1" + [ -f "$CONFIG" ] || die "no config at $CONFIG — nothing to edit" + + local mark + mark="$(log_mark "$LOG")" + + say "probe" + local port + port="$(resolve_port "$CONFIG")" + if daemon_listening "$port"; then + ok "daemon appears to be listening on 127.0.0.1:$port" + else + warn "no daemon detected listening on 127.0.0.1:$port — a verdict may never appear" + fi + + say "backup" + local backup + backup="$(backup_config "$CONFIG")" + ok "backup: $backup" + + say "candidate" + local cand + cand="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ + || die "could not create a candidate temp file next to $CONFIG" + if ! "$builder" "$cand"; then + rm -f "$cand" + die "could not build the candidate — nothing was installed. The live config at $CONFIG is unchanged." + fi + + if ! parse_check "$cand"; then + rm -f "$cand" + die "candidate does not parse as valid YAML — nothing was installed. The live config at $CONFIG is unchanged." + fi + ok "candidate parses" + + say "change (redacted)" + diff -u "$backup" "$cand" | redact || true + + say "install" + install_candidate "$cand" "$CONFIG" \ + || die "could not install the candidate onto $CONFIG — the live config was NOT changed. The validated candidate is sitting at $cand; investigate before retrying." + ok "installed: $CONFIG" + + local rc=0 + report_outcome "$mark" "$backup" || rc=$? + return "$rc" +} + +dry_run_diff() { + local builder="$1" + [ -f "$CONFIG" ] || die "no config at $CONFIG — nothing to diff against" + local cand + cand="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ + || die "could not create a candidate temp file next to $CONFIG" + if ! "$builder" "$cand"; then + rm -f "$cand" + die "could not build the candidate — this was a --dry-run, nothing would have been installed either" + fi + if ! parse_check "$cand"; then + rm -f "$cand" + die "candidate does not parse as valid YAML — this was a --dry-run, nothing would have been installed either" + fi + say "dry run — diff (redacted), nothing installed" + diff -u "$CONFIG" "$cand" | redact || true + rm -f "$cand" + return 0 +} + +restore_mode() { + [ -f "$CONFIG" ] || die "no config at $CONFIG to restore onto" + local backup + backup="$(newest_backup "$CONFIG")" + [ -n "$backup" ] || die "no backup found matching ${CONFIG}.bak.* — nothing to restore" + [ -f "$backup" ] || die "backup candidate $backup vanished" + + say "restore" + ok "restoring $backup onto $CONFIG" + local mark + mark="$(log_mark "$LOG")" + cp "$backup" "$CONFIG" || die "could not copy $backup onto $CONFIG" + ok "installed: $CONFIG" + + local rc=0 + report_outcome "$mark" "$backup" || rc=$? + return "$rc" +} + +# -------------------------------------------------------------------------------------- dispatch + +if [ -n "$WAIT_SECONDS_OVERRIDE" ]; then + WAIT_SECONDS="$WAIT_SECONDS_OVERRIDE" +else + WAIT_SECONDS="$(default_wait_seconds "$LOG")" +fi + +RC=0 +case "$MODE" in + check) + check_mode || RC=$? + ;; + set) + [ "${#SETS[@]}" -gt 0 ] || die "--set requires at least one =" + if [ "$DRY_RUN" = 1 ]; then + dry_run_diff build_from_set || RC=$? + else + run_edit build_from_set || RC=$? + fi + ;; + from) + [ -n "$FROM_FILE" ] || die "--from requires a candidate file path" + if [ "$DRY_RUN" = 1 ]; then + dry_run_diff build_from_file || RC=$? + else + run_edit build_from_file || RC=$? + fi + ;; + restore) + restore_mode || RC=$? + ;; +esac + +exit "$RC" diff --git a/scripts/test-config-edit.sh b/scripts/test-config-edit.sh new file mode 100755 index 0000000..a6e5b4c --- /dev/null +++ b/scripts/test-config-edit.sh @@ -0,0 +1,285 @@ +#!/usr/bin/env bash +# Self-contained checks for scripts/config-edit.sh — fleetd ticket #635. +# +# Drives the REAL config-edit.sh as a subprocess against a FIXTURE config and a FIXTURE log in a +# throwaway temp directory this file creates and removes. Never touches fleetd/fleetd.yaml or +# fleetd/fleetd.out, and never starts, stops, or contacts a daemon — there is no daemon here, so +# each test PLAYS the daemon: it starts config-edit.sh in the background (it is waiting on the +# log), appends the verdict line it wants, then collects the real exit code. + +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +EDIT="$ROOT/scripts/config-edit.sh" +TMP="$(mktemp -d "$ROOT/.config-edit-test.XXXXXX")" +trap 'rm -rf "$TMP"' EXIT + +fail() { + printf 'FAIL: %s\n' "$*" >&2 + return 1 +} + +assert_equals() { + local expected="$1" actual="$2" description="$3" + [ "$expected" = "$actual" ] || fail "$description: expected $expected, got $actual" +} + +assert_contains() { + local needle="$1" text="$2" description="$3" + printf '%s' "$text" | grep -qF -- "$needle" || fail "$description: missing [$needle]" +} + +assert_not_contains() { + local needle="$1" text="$2" description="$3" + if printf '%s' "$text" | grep -qF -- "$needle"; then + fail "$description: must NOT contain [$needle], but it does" + fi + return 0 +} + +# A fresh fixture pair per test: $1/fleetd.yaml (the config) and $1/fleetd.out (the log), plus a +# small wait-seconds budget so no test takes long. Returns the fixture dir via stdout. +new_fixture() { + local dir + dir="$(mktemp -d "$TMP/fixture.XXXXXX")" + cat > "$dir/fleetd.yaml" <<'YAML' +bind: + host: 127.0.0.1 + port: 19999 +broker: + uri: amqp://user:hunter2@host/vhost +profiles: + sonnet: + weight: 3 +YAML + : > "$dir/fleetd.out" + printf '%s' "$dir" +} + +# Runs config-edit.sh in the background against $dir's fixtures, with the given extra args, and +# a short --wait-seconds. Sets RUN_PID. Caller appends to $dir/fleetd.out (or not, for the +# silence test) and then calls collect_run to block for the exit code. +start_run() { + local dir="$1" wait_s="$2"; shift 2 + ( + # config-edit.sh deliberately exits 3/4/5 on several of these tests. This subshell inherits + # the parent's `set -e`, and without disabling it here the FIRST nonzero exit would kill the + # subshell before the `echo $? > rc` line ever ran — the real code would never reach the file. + set +e + "$EDIT" --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds "$wait_s" "$@" \ + > "$dir/stdout.log" 2>&1 + echo $? > "$dir/rc" + ) & + RUN_PID=$! +} + +collect_run() { + local dir="$1" + # wait echoes back the backgrounded subshell's own exit status (here, deliberately 3/4/5 on + # several tests) — under `set -e` a bare nonzero `wait` would abort this whole test script, so + # it is neutralized with `|| true`; the real code is read from $dir/rc right after. + wait "$RUN_PID" || true + RUN_OUTPUT="$(cat "$dir/stdout.log")" + RUN_RC="$(cat "$dir/rc")" +} + +# -------------------------------------------------------------- acceptance criterion 1: refusal +test_refusal_restores_byte_for_byte() { + local dir + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + + start_run "$dir" 5 --set '.broker.uri=amqp://changed@host/x' + sleep 1 + printf 'config reload refused — these keys cannot change under a running daemon: broker. Restart fleetd to apply them.\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 4 "$RUN_RC" "refusal exit code" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "refusal must restore the config byte for byte onto the pre-edit backup" +} + +# -------------------------------------------------------------- acceptance criterion 2: clean +test_clean_reload_keeps_the_edit() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=7' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "clean reload exit code" + assert_equals "7" "$(yq eval '.profiles.sonnet.weight' "$dir/fleetd.yaml")" "clean reload live value" +} + +# ----------------------------------------------------- acceptance criterion 3: deferred != clean +test_deferred_reload_is_told_apart_from_clean() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=9' + sleep 1 + printf 'config reloaded; these changes need a restart to take effect: profiles\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 3 "$RUN_RC" "deferred reload exit code" + [ "$RUN_RC" != 0 ] || fail "deferred reload must not report exit 0" + assert_contains "profiles" "$RUN_OUTPUT" "deferred reload names the key" + assert_contains "restart" "$RUN_OUTPUT" "deferred reload says a restart is needed" +} + +# -------------------------------------------------------------- acceptance criterion 4: silence +test_silence_is_its_own_answer() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 2 --set '.profiles.sonnet.weight=11' + # Feed the log nothing. + collect_run "$dir" + + assert_equals 5 "$RUN_RC" "silence exit code" + assert_equals "11" "$(yq eval '.profiles.sonnet.weight' "$dir/fleetd.yaml")" "the edited value must still be on disk" + assert_contains '--restore' "$RUN_OUTPUT" "silence prints the --restore command" + + local backup restore_cmd + backup="$(ls -t "$dir"/fleetd.yaml.bak.* | head -1)" + [ -n "$backup" ] || fail "silence must still have taken a backup" + + restore_cmd="$(printf '%s\n' "$RUN_OUTPUT" | grep -F -- '--restore --config' | sed -E 's/^[[:space:]]*//')" + [ -n "$restore_cmd" ] || fail "could not find the printed --restore invocation in the output" + # Running this --restore invocation installs the backup, then itself waits for a confirming + # verdict that this fixture never feeds — so it legitimately exits 5 ("cannot tell") here, same + # as any edit with no daemon on the other end. Only a usage/internal error (1 or 2) is a real + # failure of the command itself; the actual assertion is the byte-for-byte cmp below. + local restore_rc=0 + eval "$restore_cmd" > "$dir/restore.log" 2>&1 || restore_rc=$? + case "$restore_rc" in + 0|3|4|5) : ;; + *) fail "the printed --restore command errored out (exit $restore_rc): $(cat "$dir/restore.log")" ;; + esac + + cmp -s "$dir/fleetd.yaml" "$backup" \ + || fail "running the printed --restore command must put the file back to the original backup" +} + +# --------------------------------------------------------- acceptance criterion 5: bad candidate +test_broken_candidate_never_reaches_live_path() { + local dir rc=0 + dir="$(new_fixture)" + printf 'foo: [unclosed\n' > "$dir/broken.yaml" + + "$EDIT" --from "$dir/broken.yaml" --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 || rc=$? + + [ "$rc" -ne 0 ] || fail "a broken --from candidate must exit non-zero" + cmp -s "$dir/fleetd.yaml" <(new_fixture_yaml) \ + || fail "the broken candidate must never reach the live fixture config" +} +new_fixture_yaml() { + cat <<'YAML' +bind: + host: 127.0.0.1 + port: 19999 +broker: + uri: amqp://user:hunter2@host/vhost +profiles: + sonnet: + weight: 3 +YAML +} + +# -------------------------------------------------------------------- acceptance criterion 6 +test_marker_skips_lines_before_it() { + local dir + dir="$(new_fixture)" + printf 'config reload refused — something ancient\n' > "$dir/fleetd.out" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=5' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "a stale refusal before the marker must not be read as this edit's verdict" +} + +# ------------------------------------------------------------------- acceptance criterion 7 +test_redaction_holds() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=4' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "redaction-case reload exit code" + assert_not_contains "hunter2" "$RUN_OUTPUT" "full output must never contain the password" + assert_not_contains "user:" "$RUN_OUTPUT" "full output must never contain the userinfo" +} + +# dry-run must never touch the live file and must still redact. +test_dry_run_never_installs_and_redacts() { + local dir before + dir="$(new_fixture)" + before="$(cat "$dir/fleetd.yaml")" + + "$EDIT" --dry-run --set '.profiles.sonnet.weight=99' \ + --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 + local rc=$? + RUN_OUTPUT="$(cat "$dir/stdout.log")" + + assert_equals 0 "$rc" "dry-run exit code" + assert_equals "$before" "$(cat "$dir/fleetd.yaml")" "dry-run must never write the live config" + assert_not_contains "hunter2" "$RUN_OUTPUT" "dry-run diff must also be redacted" + assert_contains "99" "$RUN_OUTPUT" "dry-run diff must show the candidate value" +} + +# --check is read-only and always exits 0, even against a dead "daemon". +test_check_is_read_only_and_exits_zero() { + local dir before rc=0 + dir="$(new_fixture)" + before="$(cat "$dir/fleetd.yaml")" + + "$EDIT" --check --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" \ + > "$dir/stdout.log" 2>&1 || rc=$? + + assert_equals 0 "$rc" "--check exit code" + assert_equals "$before" "$(cat "$dir/fleetd.yaml")" "--check must never modify the config" +} + +test_refusal_shape_from_parse_failure_wording_is_recognised() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=6' + sleep 1 + printf 'config reload from %s refused, keeping the running config: boom\n' "$dir/fleetd.yaml" >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 4 "$RUN_RC" "the parse-failure refusal shape must also exit 4, not be read as silence" +} + +echo "== acceptance criterion 1: refusal restores byte for byte ==" +test_refusal_restores_byte_for_byte +echo "== acceptance criterion 2: clean reload keeps the edit ==" +test_clean_reload_keeps_the_edit +echo "== acceptance criterion 3: deferred reload told apart from clean ==" +test_deferred_reload_is_told_apart_from_clean +echo "== acceptance criterion 4: silence is its own answer ==" +test_silence_is_its_own_answer +echo "== acceptance criterion 5: broken candidate never reaches the live path ==" +test_broken_candidate_never_reaches_live_path +echo "== acceptance criterion 6: the marker works ==" +test_marker_skips_lines_before_it +echo "== acceptance criterion 7: the redaction holds ==" +test_redaction_holds +echo "== extra: dry-run never installs, and redacts ==" +test_dry_run_never_installs_and_redacts +echo "== extra: --check is read-only and always exits 0 ==" +test_check_is_read_only_and_exits_zero +echo "== extra: the parse-failure refusal shape is also recognised ==" +test_refusal_shape_from_parse_failure_wording_is_recognised + +printf 'PASS: config-edit acceptance criteria\n' From 4eb720029c560e7a727ff0b8f3aa2cc2c8fffcc1 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 1 Oct 2026 18:07:12 +0200 Subject: [PATCH 2/4] fleetd #635 follow-up: refuse empty --set values, gitignore backups, preserve file mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .gitignore | 7 ++ fleetd/.gitignore | 7 ++ scripts/config-edit.sh | 129 +++++++++++++++++++++++++++++---- scripts/test-config-edit.sh | 140 +++++++++++++++++++++++++++++++++++- 4 files changed, 265 insertions(+), 18 deletions(-) diff --git a/.gitignore b/.gitignore index 1aab090..04abae2 100644 --- a/.gitignore +++ b/.gitignore @@ -20,6 +20,13 @@ fleetd.out fleetd/fleetd.out logs/ +# fleetd #635 follow-up — scripts/config-edit.sh's backup directory. No leading slash, so this is +# ignored at every depth: the real one lives under fleetd/ (also named in fleetd/.gitignore, next +# to the config it backs up), and scripts/test-config-edit.sh's own throwaway fixtures build one +# under the repo root while the suite runs. --config can point anywhere, so the directory name is +# ignored everywhere rather than only where the live daemon happens to use it. +.config-backups/ + # fleetd #480: the lead rollover handover file. `leadRollover.handoverPath` points here, and the # outgoing lead rewrites it on every rollover. It is a snapshot of one moment's live state — # unpushed branches, running builds, open questions — so it is stale the moment it is written and diff --git a/fleetd/.gitignore b/fleetd/.gitignore index 5b6d1fa..5a25259 100644 --- a/fleetd/.gitignore +++ b/fleetd/.gitignore @@ -7,6 +7,13 @@ dependency-reduced-pom.xml fleetd.yaml bridged.yaml +# fleetd #635 follow-up — scripts/config-edit.sh's backups of fleetd.yaml. A backup of a file +# that must never be committed inherits that requirement. The directory is the real protection +# (it keeps working even if the backup naming changes); the glob is a backstop for a stray +# backup written the old way, directly beside fleetd.yaml, or by an older copy of the script. +.config-backups/ +fleetd.yaml.bak.* + # CB-505 audit trail + daemon stdout/stderr — runtime records, never source logs/ diff --git a/scripts/config-edit.sh b/scripts/config-edit.sh index 2c9ef22..604cf7a 100755 --- a/scripts/config-edit.sh +++ b/scripts/config-edit.sh @@ -46,6 +46,13 @@ # scripts/config-edit.sh --dry-run --set = # scripts/config-edit.sh --restore # +# `--set .a.b=` (an empty value — a forgotten typo) is REFUSED, not accepted as "clear the +# field": a null value falls back to its default rather than erroring, which is silent, not +# safe. To clear a key on purpose, write a literal null: `--set .a.b=null`. Every other value +# is always written as a YAML string (via yq's strenv(), never spliced into the expression), so +# there is currently no --set spelling for the literal three-character STRING "null" itself — use +# --from for that rare case. +# # Overrides (so this is drivable with no daemon — see scripts/test-config-edit.sh): # --config default: fleetd/fleetd.yaml # --log default: fleetd/fleetd.out @@ -77,6 +84,16 @@ DRY_RUN=0 SETS=() FROM_FILE="" +# fleetd #635 follow-up — a signal (or any early exit while a candidate is still uninstalled) must +# not leave a `.config-edit.XXXXXX` file sitting beside the live config forever. CAND is global +# (never a function-local) on purpose: this ONE trap, set once, covers every path that ever +# creates a candidate — run_edit and dry_run_diff both assign it, and clear it back to "" once the +# file is consumed (installed, or explicitly removed), so a later, unrelated exit never retries a +# path that already served its purpose. +CAND="" +cleanup_candidate() { [ -n "$CAND" ] && rm -f "$CAND" 2>/dev/null; return 0; } +trap cleanup_candidate EXIT INT TERM + say() { printf '\n\033[1m== %s\033[0m\n' "$*"; } ok() { printf ' ok %s\n' "$*"; } warn() { printf ' WARN %s\n' "$*"; } @@ -114,7 +131,7 @@ while [ $# -gt 0 ]; do --wait-seconds) [ $# -ge 2 ] || die "--wait-seconds requires a number of seconds" WAIT_SECONDS_OVERRIDE="$2"; shift 2 ;; - -h|--help) sed -n '3,63p' "$SELF"; exit 0 ;; + -h|--help) sed -n '3,70p' "$SELF"; exit 0 ;; *) echo "unknown option: $1 (try --help)" >&2; exit 2 ;; esac done @@ -272,18 +289,62 @@ default_wait_seconds() { # # Timestamped, never pruned — "keep backups" per the ticket. A pid suffix avoids a same-second # collision between two invocations. +# +# fleetd #635 follow-up — lands under a DEDICATED, gitignored directory beside the config +# (/.config-backups/), never beside the config file itself. The whole reason fleetd.yaml is +# gitignored is that it must never be committed, and a backup of it inherits that requirement — a +# bare `fleetd.yaml.bak.*` next to a tracked directory is one `git add -A`/`git add .` away from +# committing the live config. A directory beats a glob on its own: the glob only protects today's +# naming, a location keeps working even if the naming changes later. (See .gitignore for the glob +# kept anyway, as a backstop for a stray backup written the old way.) +BACKUP_DIRNAME=".config-backups" + +backup_dir_for() { + local src="$1" + printf '%s/%s' "$(dirname "$src")" "$BACKUP_DIRNAME" +} + backup_config() { - local src="$1" ts backup + local src="$1" ts backup dir base + dir="$(backup_dir_for "$src")" + mkdir -p "$dir" \ + || die "could not create the backup directory $dir — refusing to edit without a backup. The live config at $src was NOT touched." ts="$(date -u +%Y%m%dT%H%M%S)Z" - backup="${src}.bak.${ts}.$$" + base="$(basename "$src")" + backup="${dir}/${base}.bak.${ts}.$$" cp "$src" "$backup" \ || die "could not create a backup at $backup — refusing to edit without one. The live config at $src was NOT touched." printf '%s' "$backup" } newest_backup() { - local cfg="$1" - ls -t "${cfg}".bak.* 2>/dev/null | head -1 || true + local cfg="$1" dir base + dir="$(backup_dir_for "$cfg")" + base="$(basename "$cfg")" + ls -t "${dir}/${base}".bak.* 2>/dev/null | head -1 || true +} + +# ------------------------------------------------------------------------------------ the file mode +# +# fleetd #635 follow-up — `mv` from a mktemp candidate carries mktemp's 0600 onto the live path +# forever (measured: 644 -> 600 after one --set), and a restore does not undo it either, because +# `cp` onto an EXISTING file keeps the DESTINATION's mode, not the source's. Capture the live +# file's mode before anything touches it, and reapply it to whatever lands on that path +# afterwards — the candidate before install, and the config again after a restore — so an edit +# changes the file's CONTENT only, never its permissions. BSD `stat -f '%Lp'` first (matches this +# project's dev machine), GNU `stat -c '%a'` as the fallback. Prints nothing when the file does +# not exist yet, so apply_mode then does nothing and a first-ever edit falls back to the normal +# umask default rather than inventing a number. +file_mode() { + local file="$1" + [ -f "$file" ] || return 0 + stat -f '%Lp' "$file" 2>/dev/null || stat -c '%a' "$file" 2>/dev/null || true +} + +apply_mode() { + local file="$1" mode="$2" + [ -n "$mode" ] || return 0 + chmod "$mode" "$file" 2>/dev/null || true } # --------------------------------------------------------------------------- candidate builders @@ -291,6 +352,23 @@ newest_backup() { # Never edit the live file in place. Each builder fills $1 (a temp file already sitting in the # SAME directory as the live config, so the later `mv` install is a rename, not a cross-device # copy — see run_edit). +# fleetd #635 follow-up — a forgotten value (`--set .a.b=`, a plausible typo) must never be +# accepted as "clear the field". `*=*` alone cannot tell "--set .a.b=" from "--set .a.b=7" apart +# — both contain an `=` — so the guard has to look at the VALUE, not the shape of the argument. +# An empty value refuses outright: nothing is installed, and the message names the likely cause +# AND the two ways to actually mean it (clear on purpose, or an intentional empty string via +# --from). Measured against the real daemon loader: a quoted empty string reads back as a null +# field (`quoted empty -> OK int=null`), and a null numeric field FALLS BACK TO ITS DEFAULT rather +# than erroring — so this is not a cosmetic nit, it is the one shape of edit that widens capacity +# silently instead of failing loudly, which is exactly what this script exists to catch. +# +# A deliberate clear needs its own spelling, because `""` and YAML `null` are NOT the same value +# to the loader (`""` is a valid empty String; `null` means absent, and an Integer field reads +# either the same way — null — but a String field would keep `""` as a real value). `--set +# .a.b=null` is that spelling: it writes a literal, unquoted `null` via yq, never the string +# "null" through strenv(). One consequence worth knowing: there is currently no --set spelling +# for the three-character STRING "null" itself (it collides with the clear spelling) — use +# --from for that rare case. apply_set_pairs() { local cand="$1" kv path value shift @@ -302,6 +380,17 @@ apply_set_pairs() { path="${kv%%=*}" path="${path#.}" value="${kv#*=}" + if [ -z "$value" ]; then + die "--set '$kv' has an EMPTY value — refusing. Nothing was installed. A forgotten value + would NULL the field, and a null value falls back to its default rather than erroring — + silent, not safe. Did you mean --set .${path}=null to clear it on purpose, or --from a + file if you need a genuinely empty string?" + fi + if [ "$value" = "null" ]; then + yq eval -i ".${path} = null" "$cand" \ + || die "yq could not clear '$kv' — nothing was installed. The live config is unchanged." + continue + fi CONFIG_EDIT_SET_VALUE="$value" yq eval -i ".${path} = strenv(CONFIG_EDIT_SET_VALUE)" "$cand" \ || die "yq could not apply --set '$kv' — nothing was installed. The live config is unchanged." done @@ -340,10 +429,12 @@ restore_command_line() { # restore itself reloaded cleanly. Never claims a restore it did not observe — if the second wait # also times out, it says so plainly rather than reporting "restored" as though confirmed. restore_and_confirm() { - local backup="$1" mark2 + local backup="$1" mark2 orig_mode + orig_mode="$(file_mode "$CONFIG")" mark2="$(log_mark "$LOG")" cp "$backup" "$CONFIG" \ || die "could not restore $backup onto $CONFIG — the live config is left as the REFUSED edit. Fix this by hand immediately: cp \"$backup\" \"$CONFIG\"" + apply_mode "$CONFIG" "$orig_mode" ok "restored from $backup" if wait_for_verdict "$LOG" "$mark2" "$WAIT_SECONDS"; then case "$VERDICT_KIND" in @@ -448,8 +539,9 @@ run_edit() { local builder="$1" [ -f "$CONFIG" ] || die "no config at $CONFIG — nothing to edit" - local mark + local mark orig_mode mark="$(log_mark "$LOG")" + orig_mode="$(file_mode "$CONFIG")" say "probe" local port @@ -467,25 +559,29 @@ run_edit() { say "candidate" local cand - cand="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ + CAND="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ || die "could not create a candidate temp file next to $CONFIG" + cand="$CAND" if ! "$builder" "$cand"; then - rm -f "$cand" + rm -f "$cand"; CAND="" die "could not build the candidate — nothing was installed. The live config at $CONFIG is unchanged." fi if ! parse_check "$cand"; then - rm -f "$cand" + rm -f "$cand"; CAND="" die "candidate does not parse as valid YAML — nothing was installed. The live config at $CONFIG is unchanged." fi ok "candidate parses" + apply_mode "$cand" "$orig_mode" + say "change (redacted)" diff -u "$backup" "$cand" | redact || true say "install" install_candidate "$cand" "$CONFIG" \ || die "could not install the candidate onto $CONFIG — the live config was NOT changed. The validated candidate is sitting at $cand; investigate before retrying." + CAND="" ok "installed: $CONFIG" local rc=0 @@ -497,19 +593,20 @@ dry_run_diff() { local builder="$1" [ -f "$CONFIG" ] || die "no config at $CONFIG — nothing to diff against" local cand - cand="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ + CAND="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \ || die "could not create a candidate temp file next to $CONFIG" + cand="$CAND" if ! "$builder" "$cand"; then - rm -f "$cand" + rm -f "$cand"; CAND="" die "could not build the candidate — this was a --dry-run, nothing would have been installed either" fi if ! parse_check "$cand"; then - rm -f "$cand" + rm -f "$cand"; CAND="" die "candidate does not parse as valid YAML — this was a --dry-run, nothing would have been installed either" fi say "dry run — diff (redacted), nothing installed" diff -u "$CONFIG" "$cand" | redact || true - rm -f "$cand" + rm -f "$cand"; CAND="" return 0 } @@ -522,9 +619,11 @@ restore_mode() { say "restore" ok "restoring $backup onto $CONFIG" - local mark + local mark orig_mode mark="$(log_mark "$LOG")" + orig_mode="$(file_mode "$CONFIG")" cp "$backup" "$CONFIG" || die "could not copy $backup onto $CONFIG" + apply_mode "$CONFIG" "$orig_mode" ok "installed: $CONFIG" local rc=0 diff --git a/scripts/test-config-edit.sh b/scripts/test-config-edit.sh index a6e5b4c..20bd1cd 100755 --- a/scripts/test-config-edit.sh +++ b/scripts/test-config-edit.sh @@ -51,6 +51,7 @@ broker: profiles: sonnet: weight: 3 + maxLoad: 5 YAML : > "$dir/fleetd.out" printf '%s' "$dir" @@ -143,7 +144,7 @@ test_silence_is_its_own_answer() { assert_contains '--restore' "$RUN_OUTPUT" "silence prints the --restore command" local backup restore_cmd - backup="$(ls -t "$dir"/fleetd.yaml.bak.* | head -1)" + backup="$(ls -t "$dir"/.config-backups/fleetd.yaml.bak.* | head -1)" [ -n "$backup" ] || fail "silence must still have taken a backup" restore_cmd="$(printf '%s\n' "$RUN_OUTPUT" | grep -F -- '--restore --config' | sed -E 's/^[[:space:]]*//')" @@ -186,6 +187,7 @@ broker: profiles: sonnet: weight: 3 + maxLoad: 5 YAML } @@ -203,7 +205,14 @@ test_marker_skips_lines_before_it() { assert_equals 0 "$RUN_RC" "a stale refusal before the marker must not be read as this edit's verdict" } -# ------------------------------------------------------------------- acceptance criterion 7 +# ------------------------------------------------------------------- acceptance criterion 7 (+13) +# fleetd #635 follow-up (ticket comment 17659) — the two assertions below this comment were the +# WHOLE test before the follow-up, and both are negative-only: they pass just as happily when the +# diff is never printed at all as when it is printed and correctly redacted. A mutant that deletes +# `diff -u "$backup" "$cand" | redact` from the edit path survives them, because an absent output +# contains neither "hunter2" nor "user:" either — see the mutation-and-revert proof in the reply. +# Criterion 13 is the fix: a LOUD positive control that only passes when a diff was demonstrably +# printed AND the redaction demonstrably ran on real content, not merely that nothing leaked. test_redaction_holds() { local dir dir="$(new_fixture)" @@ -216,6 +225,121 @@ test_redaction_holds() { assert_equals 0 "$RUN_RC" "redaction-case reload exit code" assert_not_contains "hunter2" "$RUN_OUTPUT" "full output must never contain the password" assert_not_contains "user:" "$RUN_OUTPUT" "full output must never contain the userinfo" + # acceptance criterion 13 — positive control: the diff's default 3-line context around the + # changed "weight" key also covers the fixture's "uri:" line, so a genuinely-printed, genuinely- + # redacted diff must contain BOTH the redaction marker and the changed key's name. A test that + # only ever asserts absence cannot tell "redacted" from "never printed" apart; this can. + assert_contains "" "$RUN_OUTPUT" "the redaction must be PROVEN to have run on real content, not merely absent" + assert_contains "weight" "$RUN_OUTPUT" "a diff must have been demonstrably printed at all" +} + +# ------------------------------------------------------- acceptance criterion 9: forgotten value +# `--set .a.b=` is a plausible typo (the value simply forgotten), and it must be refused outright +# rather than silently nulling the field — a null numeric field falls back to its default, which +# widens capacity instead of failing loudly. No background verdict feeder here: a refused --set +# must never even reach the daemon, so this never starts a background run at all. +test_forgotten_value_refuses_and_installs_nothing() { + local dir rc=0 + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + + "$EDIT" --set '.profiles.sonnet.maxLoad=' \ + --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 || rc=$? + RUN_OUTPUT="$(cat "$dir/stdout.log")" + + [ "$rc" -ne 0 ] || fail "an empty --set value must exit non-zero, got 0" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "an empty --set value must install nothing — the live fixture changed" + assert_contains "EMPTY value" "$RUN_OUTPUT" "the refusal must name the empty value" +} + +# ---------------------------------------------------------- acceptance criterion 10: explicit null +# `--set .a.b=null` is the deliberate-clear spelling, and it must write a REAL yaml null, never +# the string "''" — those are different values to the daemon's loader (fleetd ticket #635's +# follow-up comment measured `""` reading back as a null field anyway, which is exactly why the +# two forms must not collapse onto each other: `--set path=` refuses instead of silently reaching +# this same null outcome through the back door). Read the RAW line with grep, never only through +# `yq` — `yq eval` reports `null` for both an actual null and a missing/absent key, so it cannot +# tell "wrote null" apart from "wrote nothing"; only the literal line on disk can. +test_explicit_null_writes_bare_null_not_empty_string() { + local dir + dir="$(new_fixture)" + + start_run "$dir" 5 --set '.profiles.sonnet.maxLoad=null' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "explicit null clear exit code" + local raw_line + raw_line="$(grep -E 'maxLoad' "$dir/fleetd.yaml")" + assert_contains "null" "$raw_line" "the installed line must spell a bare null" + assert_not_contains '""' "$raw_line" "the installed line must NOT be a quoted empty string" +} + +# ------------------------------------------------------- acceptance criterion 11: backup never committable +# A backup of fleetd.yaml inherits fleetd.yaml's own "never commit this" requirement (fleetd #635 +# follow-up, ticket comment 17655). Proves two things: the backup lands somewhere `git +# check-ignore` reports as ignored (equivalently, a path `git status --porcelain` never lists as +# untracked), AND that --restore still finds and uses it from that location. +test_backup_is_never_committable() { + local dir backup + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + + start_run "$dir" 5 --set '.profiles.sonnet.weight=55' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "setup edit exit code" + + 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?" + + git -C "$ROOT" check-ignore -q -- "$backup" \ + || fail "the backup at $backup is NOT gitignored — it would survive a git add -A" + if git -C "$ROOT" status --porcelain -- "$backup" 2>/dev/null | grep -q '^??'; then + fail "git status still lists the backup as untracked: $backup" + fi + + start_run "$dir" 5 --restore + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "--restore after the backup-location change exit code" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "--restore from the new backup location must still put the file back byte for byte" +} + +# --------------------------------------------------------- acceptance criterion 12: file mode +# `mv` from a mktemp candidate carries mktemp's 0600 forever, and a plain `cp` onto an existing +# file keeps the DESTINATION's mode rather than the source's, so a restore does not undo the +# narrowing either (fleetd #635 follow-up, ticket comment 17657). Proves the mode survives an edit +# AND a subsequent restore, from two different starting points — 644 is the common case, 600 +# proves the fix PRESERVES whatever mode was there rather than hardcoding 644. +test_file_mode_survives_edit_and_restore() { + local dir want got + for want in 644 600; do + dir="$(new_fixture)" + chmod "$want" "$dir/fleetd.yaml" + + start_run "$dir" 5 --set ".profiles.sonnet.weight=${want}" + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "mode-preservation setup edit exit code ($want)" + got="$(stat -f '%Lp' "$dir/fleetd.yaml" 2>/dev/null || stat -c '%a' "$dir/fleetd.yaml")" + assert_equals "$want" "$got" "mode must survive a --set ($want)" + + start_run "$dir" 5 --restore + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "mode-preservation restore exit code ($want)" + got="$(stat -f '%Lp' "$dir/fleetd.yaml" 2>/dev/null || stat -c '%a' "$dir/fleetd.yaml")" + assert_equals "$want" "$got" "mode must survive a --restore ($want)" + done } # dry-run must never touch the live file and must still redact. @@ -234,6 +358,8 @@ test_dry_run_never_installs_and_redacts() { assert_equals "$before" "$(cat "$dir/fleetd.yaml")" "dry-run must never write the live config" assert_not_contains "hunter2" "$RUN_OUTPUT" "dry-run diff must also be redacted" assert_contains "99" "$RUN_OUTPUT" "dry-run diff must show the candidate value" + # Same positive-control reasoning as acceptance criterion 13, applied to the dry-run diff path. + assert_contains "" "$RUN_OUTPUT" "the dry-run diff's redaction must be PROVEN to have run, not merely absent" } # --check is read-only and always exits 0, even against a dead "daemon". @@ -273,8 +399,16 @@ echo "== acceptance criterion 5: broken candidate never reaches the live path == test_broken_candidate_never_reaches_live_path echo "== acceptance criterion 6: the marker works ==" test_marker_skips_lines_before_it -echo "== acceptance criterion 7: the redaction holds ==" +echo "== acceptance criterion 7 (+13: redaction is proven to have run) ==" test_redaction_holds +echo "== acceptance criterion 9: a forgotten value refuses and installs nothing ==" +test_forgotten_value_refuses_and_installs_nothing +echo "== acceptance criterion 10: an explicit clear writes a bare null ==" +test_explicit_null_writes_bare_null_not_empty_string +echo "== acceptance criterion 11: a backup is never committable ==" +test_backup_is_never_committable +echo "== acceptance criterion 12: the file mode survives an edit and a restore ==" +test_file_mode_survives_edit_and_restore echo "== extra: dry-run never installs, and redacts ==" test_dry_run_never_installs_and_redacts echo "== extra: --check is read-only and always exits 0 ==" From 096f08c866da13dca75ca9963d3fd1c702370a68 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 1 Oct 2026 18:25:08 +0200 Subject: [PATCH 3/4] fleetd #635 follow-up: fix the stale --restore not-found message (defect 6, criterion 14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- scripts/config-edit.sh | 17 +++++++++++++-- scripts/test-config-edit.sh | 42 +++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/scripts/config-edit.sh b/scripts/config-edit.sh index 604cf7a..52e4c0f 100755 --- a/scripts/config-edit.sh +++ b/scripts/config-edit.sh @@ -612,9 +612,22 @@ dry_run_diff() { restore_mode() { [ -f "$CONFIG" ] || die "no config at $CONFIG to restore onto" - local backup + local backup dir base backup="$(newest_backup "$CONFIG")" - [ -n "$backup" ] || die "no backup found matching ${CONFIG}.bak.* — nothing to restore" + if [ -z "$backup" ]; then + # fleetd #635 follow-up (ticket comment 17664) — this message must name the directory the + # code actually searches (backup_dir_for, same as newest_backup), not the old beside-the- + # config glob. A backup written the OLD way is real and NOT searched any more — say so and + # give the one-line recovery command — but do NOT make the search itself look there; that + # would be a behaviour change nobody asked for. The message is the only thing being fixed. + dir="$(backup_dir_for "$CONFIG")" + base="$(basename "$CONFIG")" + die "no backup found matching ${dir}/${base}.bak.* — nothing to restore. + A backup written the OLD way, directly beside the config (${CONFIG}.bak.*), is NOT + searched — that location was retired so a backup of a file that must never be committed + cannot sit next to a tracked directory. If one exists there, recover it by hand: + cp ${CONFIG}.bak.. $CONFIG" + fi [ -f "$backup" ] || die "backup candidate $backup vanished" say "restore" diff --git a/scripts/test-config-edit.sh b/scripts/test-config-edit.sh index 20bd1cd..138b0fa 100755 --- a/scripts/test-config-edit.sh +++ b/scripts/test-config-edit.sh @@ -342,6 +342,46 @@ test_file_mode_survives_edit_and_restore() { done } +# ----------------------------------------- acceptance criterion 14: restore message names the real directory +# fleetd #635 follow-up (ticket comment 17664, defect 6) — the --restore "no backup found" +# message used to print the OLD beside-the-config glob even though newest_backup had already +# moved to searching the managed directory. Proves BOTH directions: the not-found message names +# the directory actually searched (not merely that it says SOMETHING), and that a real backup +# sitting in that directory still lets --restore succeed — otherwise the fix could regress into +# a message that is always printed regardless of whether a backup exists. +test_restore_message_names_the_searched_directory() { + local dir rc=0 + + # Direction 1: no backup anywhere — the message must name .config-backups/, not the bare + # beside-the-config glob the OLD code printed. + dir="$(new_fixture)" + "$EDIT" --restore --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 || rc=$? + RUN_OUTPUT="$(cat "$dir/stdout.log")" + + assert_equals 1 "$rc" "--restore with no backup anywhere exit code" + assert_contains ".config-backups/fleetd.yaml.bak.*" "$RUN_OUTPUT" \ + "the not-found message must name the directory actually searched, not the old beside-the-config glob" + + # Direction 2: a real backup IS present in .config-backups/ — --restore must still succeed, so + # the message fix cannot have turned into one that prints regardless of whether a backup exists. + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + start_run "$dir" 5 --set '.profiles.sonnet.weight=77' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "setup edit exit code for criterion 14's second half" + + start_run "$dir" 5 --restore + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "--restore with a real backup present must still succeed" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "--restore with a real backup present must put the file back byte for byte" +} + # dry-run must never touch the live file and must still redact. test_dry_run_never_installs_and_redacts() { local dir before @@ -409,6 +449,8 @@ echo "== acceptance criterion 11: a backup is never committable ==" test_backup_is_never_committable echo "== acceptance criterion 12: the file mode survives an edit and a restore ==" test_file_mode_survives_edit_and_restore +echo "== acceptance criterion 14: the restore message names the directory actually searched ==" +test_restore_message_names_the_searched_directory echo "== extra: dry-run never installs, and redacts ==" test_dry_run_never_installs_and_redacts echo "== extra: --check is read-only and always exits 0 ==" From d7f94cafa242d83882bd5ffbb95a3a85c0cc19af Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 1 Oct 2026 18:47:00 +0200 Subject: [PATCH 4/4] fleetd #635 follow-up: redact() masks block-scalar continuations + passphrase; --set failures stop echoing the value (defects 7 and 8, criteria 15a/15b/16) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 "". 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. --- scripts/config-edit.sh | 71 +++++++++++++++++---- scripts/test-config-edit.sh | 124 ++++++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 13 deletions(-) diff --git a/scripts/config-edit.sh b/scripts/config-edit.sh index 52e4c0f..7d47d63 100755 --- a/scripts/config-edit.sh +++ b/scripts/config-edit.sh @@ -143,21 +143,58 @@ done # Two independent passes, applied to every diff this script ever prints: # 1. `scheme://user:pass@host` -> `scheme://@host`, globally (the `g` flag matters — # a line can carry more than one URI). -# 2. Any line whose key looks like TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY, matched -# case-insensitively against the key text (uriEnv, gitTokenEnv, ... are camelCase, not -# SCREAMING_CASE) has its whole value blanked, diff marker and indentation kept so the shape -# of the change is still visible. Deliberately conservative: a false-positive redaction on an -# unrelated line costs nothing, an unredacted secret is a security defect (acceptance -# criterion 7). +# 2. Any line whose key looks like TOKEN|SECRET|PASSWORD|PASSWD|PASSPHRASE|CREDENTIAL|URI|_KEY, +# matched case-insensitively against the key text (uriEnv, gitTokenEnv, ... are camelCase, +# not SCREAMING_CASE) has its whole value blanked, diff marker and indentation kept so the +# shape of the change is still visible. Deliberately conservative: a false-positive +# redaction on an unrelated line costs nothing, an unredacted secret is a security defect +# (acceptance criterion 7). +# +# fleetd #635 follow-up (ticket comment 17670, defect 7) — a masked key line is not the whole +# story: a YAML block scalar (`|`, `|-`, `>`, `>-`, ...) puts the VALUE on the lines that follow +# the key, each indented deeper than it. The key-name match above only ever sees the key line +# itself, so those continuation lines used to flow straight through unredacted while the key line +# right above them printed a reassuring "" — an incomplete redactor that looks complete +# is worse than one that visibly does nothing, because it stops a reviewer from looking further. +# The fix is structural, not another name to match: once a key line is masked, every following +# line indented STRICTLY DEEPER than that key is masked too, by indentation alone, until the +# indentation returns to the key's own level or shallower. This needs no knowledge of the key's +# name and so protects a block scalar under any masked key, present or future. +# +# `redact` is always fed `diff -u` output, and every line of a unified diff starts with exactly +# one of ' ', '+', '-' (the three body markers; '@'/'-'/'+' for the three header-line kinds too). +# That one leading character is NOT part of the YAML indentation, and must be stripped before +# indentation is measured or a key is matched — otherwise a changed ('+' or '-') line reads one +# column shallower than it really is, and either wrongly escapes a continuation mask or wrongly +# ends one early. Tabs are out of scope: YAML forbids them for indentation, and this is a bounded +# fix, not a YAML parser. redact() { - local line marker saved_nocasematch=0 + local line prefix content indent lead key + local masked=0 masked_indent=0 saved_nocasematch=0 shopt -q nocasematch && saved_nocasematch=1 shopt -s nocasematch sed -E 's#://[^@]*@#://@#g' | while IFS= read -r line || [ -n "$line" ]; do - if [[ "$line" =~ ^([-+\ ]?[[:space:]]*[A-Za-z0-9_.-]+:) ]]; then - marker="${BASH_REMATCH[1]}" - if [[ "$marker" =~ (TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY) ]]; then - printf '%s \n' "$marker" + case "$line" in + [\ +-]*) prefix="${line:0:1}"; content="${line:1}" ;; + *) prefix=""; content="$line" ;; + esac + + indent=0 + while [ "${content:$indent:1}" = " " ]; do indent=$((indent + 1)); done + + if [ "$masked" = 1 ] && [ "$indent" -gt "$masked_indent" ]; then + printf '%s%*s\n' "$prefix" "$indent" "" + continue + fi + masked=0 + + if [[ "$content" =~ ^([[:space:]]*)([A-Za-z0-9_.-]+:) ]]; then + lead="${BASH_REMATCH[1]}" + key="${BASH_REMATCH[2]}" + if [[ "$key" =~ (TOKEN|SECRET|PASSWORD|PASSWD|PASSPHRASE|CREDENTIAL|URI|_KEY) ]]; then + printf '%s%s%s \n' "$prefix" "$lead" "$key" + masked=1 + masked_indent="$indent" continue fi fi @@ -386,13 +423,21 @@ apply_set_pairs() { silent, not safe. Did you mean --set .${path}=null to clear it on purpose, or --from a file if you need a genuinely empty string?" fi + # fleetd #635 follow-up (ticket comment 17673, defect 8) — these two failure messages used to + # echo the full "$kv" (path=value, exactly as the operator typed it), unredacted. The operator + # already has the value, so a terminal is not where this leaks — the risk is where the output + # goes NEXT: this fleet pastes command output into tickets, PRs and fleet_reply bodies, and a + # failure is exactly when someone copies it to ask for help. Print the PATH, which is what's + # needed to fix the command, and never the value. $kv is not key:value-shaped YAML, so piping + # it through redact would just pass it straight through — a false sense of coverage, the same + # mistake as defect 7. if [ "$value" = "null" ]; then yq eval -i ".${path} = null" "$cand" \ - || die "yq could not clear '$kv' — nothing was installed. The live config is unchanged." + || die "yq could not clear --set '.${path}=null' — nothing was installed. The live config is unchanged." continue fi CONFIG_EDIT_SET_VALUE="$value" yq eval -i ".${path} = strenv(CONFIG_EDIT_SET_VALUE)" "$cand" \ - || die "yq could not apply --set '$kv' — nothing was installed. The live config is unchanged." + || die "yq could not apply --set '.${path}=' — nothing was installed. The live config is unchanged." done } diff --git a/scripts/test-config-edit.sh b/scripts/test-config-edit.sh index 138b0fa..3518771 100755 --- a/scripts/test-config-edit.sh +++ b/scripts/test-config-edit.sh @@ -382,6 +382,124 @@ test_restore_message_names_the_searched_directory() { || fail "--restore with a real backup present must put the file back byte for byte" } +# ------------------------- acceptance criterion 15a: block-scalar continuation lines are redacted +# fleetd #635 follow-up (ticket comment 17670, defect 7) — redact() used to look only AT the key +# line. A YAML block scalar (`|`) puts its value on the lines that FOLLOW the key, each indented +# deeper than it, so the real secret flowed through untouched while the key line right above it +# printed a reassuring "" — worse than no redaction, because the marker stops a reader +# from looking further. The edited key here ("retries") sits directly next to the block scalar, +# well inside diff -u's default 3-line context window, so the printed hunk is GUARANTEED to +# include the secret's lines — placing the edit further away would let this pass today even +# without the fix, proving nothing (the ticket comment's own warning, from the lead's first +# reproduction attempt). The positive control runs FIRST: without it, "the secret never entered +# the diff at all" would pass identically to "it entered and was correctly redacted". +new_fixture_block_scalar() { + local dir + dir="$(mktemp -d "$TMP/fixture.XXXXXX")" + cat > "$dir/fleetd.yaml" <<'YAML' +bind: + host: 127.0.0.1 + port: 19999 +broker: + uri: amqp://user:hunter2@host/vhost +auth: + token: | + FAKELEAK-BLOCK-SCALAR + retries: 1 +profiles: + sonnet: + weight: 3 + maxLoad: 5 +YAML + : > "$dir/fleetd.out" + printf '%s' "$dir" +} + +test_block_scalar_continuation_is_redacted() { + local dir + dir="$(new_fixture_block_scalar)" + + start_run "$dir" 5 --set '.auth.retries=2' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "block-scalar case reload exit code" + # Positive control FIRST: the key's own (masked) line must really be in the printed diff, or the + # negative assertion right after proves nothing — see the comment above this test. + assert_contains "token:" "$RUN_OUTPUT" "block-scalar case: the key's line must be in the printed diff" + assert_contains "" "$RUN_OUTPUT" "block-scalar case: redaction must be proven to have run on real content" + assert_not_contains "FAKELEAK-BLOCK-SCALAR" "$RUN_OUTPUT" "block-scalar case: the block scalar's VALUE must never leak" +} + +# ------------------------------------- acceptance criterion 15b: "passphrase" is also recognised +# "passphrase" was in none of TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIAL|URI|_KEY (ticket comment +# 17670). This is a plain key:value line, not a block scalar — kept in its OWN fixture and OWN +# function, separate from criterion 15a, so that a failure in one case can never mask a failure in +# the other (a single combined test would abort under `set -e` at its first failing assertion, +# and the second case would then never even run). +new_fixture_passphrase() { + local dir + dir="$(mktemp -d "$TMP/fixture.XXXXXX")" + cat > "$dir/fleetd.yaml" <<'YAML' +bind: + host: 127.0.0.1 + port: 19999 +broker: + uri: amqp://user:hunter2@host/vhost +auth: + passphrase: FAKELEAK-PASSPHRASE + retries: 1 +profiles: + sonnet: + weight: 3 + maxLoad: 5 +YAML + : > "$dir/fleetd.out" + printf '%s' "$dir" +} + +test_passphrase_key_is_redacted() { + local dir + dir="$(new_fixture_passphrase)" + + start_run "$dir" 5 --set '.auth.retries=2' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + + assert_equals 0 "$RUN_RC" "passphrase case reload exit code" + assert_contains "passphrase:" "$RUN_OUTPUT" "passphrase case: the key's line must be in the printed diff" + assert_contains "" "$RUN_OUTPUT" "passphrase case: redaction must be proven to have run on real content" + assert_not_contains "FAKELEAK-PASSPHRASE" "$RUN_OUTPUT" "passphrase case: the passphrase VALUE must never leak" +} + +# ----------------------------------- acceptance criterion 16: a failing --set must not echo value +# fleetd #635 follow-up (ticket comment 17673, defect 8) — apply_set_pairs used to echo the FULL +# "$kv" (path=value, exactly as typed) in its yq-failure messages, so a broken --set with a +# secret-looking value printed that value right back out. The path alone is what the positive +# control proves is still there — it is what the operator needs to fix their command — and the +# negative assertion proves the value itself never appears. Kept to exactly this one failure +# shape (an invalid yq path/expression), matching the ticket's own reproduction. +test_failing_set_does_not_echo_its_value() { + local dir rc=0 + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + + "$EDIT" --dry-run --set '.broker.["bad=FAKELEAK-SETVALUE' \ + --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 || rc=$? + RUN_OUTPUT="$(cat "$dir/stdout.log")" + + [ "$rc" -ne 0 ] || fail "a --set with an invalid yq expression must exit non-zero, got 0" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "a failing --set must install nothing — the live fixture changed" + # Positive control FIRST: the path must still be in the message, or the negative assertion right + # after proves nothing (the message could simply have disappeared entirely). + assert_contains '.broker.["bad' "$RUN_OUTPUT" "the failure message must still name the PATH" + assert_not_contains "FAKELEAK-SETVALUE" "$RUN_OUTPUT" "the failure message must NEVER echo the VALUE" +} + # dry-run must never touch the live file and must still redact. test_dry_run_never_installs_and_redacts() { local dir before @@ -451,6 +569,12 @@ echo "== acceptance criterion 12: the file mode survives an edit and a restore = test_file_mode_survives_edit_and_restore echo "== acceptance criterion 14: the restore message names the directory actually searched ==" test_restore_message_names_the_searched_directory +echo "== acceptance criterion 15a: a block scalar's continuation lines are redacted ==" +test_block_scalar_continuation_is_redacted +echo "== acceptance criterion 15b: a passphrase key is also recognised ==" +test_passphrase_key_is_redacted +echo "== acceptance criterion 16: a failing --set must not echo its value ==" +test_failing_set_does_not_echo_its_value echo "== extra: dry-run never installs, and redacts ==" test_dry_run_never_installs_and_redacts echo "== extra: --check is read-only and always exits 0 =="