qa: harden blacklist test process and runner boundaries

[dborup]

QA-hardening of qa/scripts/blacklist-test.sh for the follow-ups found while verifying PR #82 (tracked in issue 83). Only the QA script, its unit tests, the QA plan text and one CI test step change; no production code, schema, privacy, MQTT or runtime behaviour.

Original findings → what changed

Finding (from #82's limitations / issue 83)	Resolution
Test pubkey in process arguments: config-edit ssh (PK=… bash -s), curl URLs, two grep patterns (9 execve per run)	Config edit sends the pubkey on ssh stdin (line after IFS= read -r PK), remote jq/python3 get it via the environment (env.PK / os.environ); curl reads its URL from a -K - config on stdin; grep reads mode-600 pattern files (grep -F -f) in the run's mode-700 temp dir. SQL was already on stdin.
ADMIN_API_TOKEN calls /api/admin/transmissions?from_node=…, which does not exist	Removed. No such endpoint exists or is planned in cmd/server; no new endpoint was added. §10.2 is SQLite-only. Setting ADMIN_API_TOKEN now refuses to start (exit 2, value never printed).
One TARGET_DB_PATH used for both runners; host sqlite3 could create an empty file	Split into TARGET_CONTAINER_DB_PATH (container runner only) and TARGET_HOST_DB_PATH (host runner only). A runner is only a candidate if its own path is set. Before any query the path must be a non-empty regular file in that runner's environment, and sqlite3 runs with -readonly — two independent guards against creating a file. Legacy TARGET_DB_PATH refuses to start (exit 2) instead of being reinterpreted.
SIGPIPE when only stderr is broken could end teardown early	trap 'teardown 141' PIPE; SIGPIPE is ignored inside teardown. See below.
Imprecise RESTART_WAIT_S wording	Now documented as the deadline for starting a new /api/stats poll; worst case ≈ RESTART_WAIT_S + CURL_TIMEOUT + 3. The second-signal note is corrected: an interrupted ssh step → teardown-failed, an interrupted stats poll is just one failed poll.
No shellcheck	CI step shellcheck -x -P SCRIPTDIR on both scripts (ubuntu-latest ships shellcheck); both are clean.

Found during this work

• bash 3.2 output replay (macOS /bin/bash): after a write to a dead stdout fails, bash 3.2 keeps the text buffered and the next subshell flushes it into its output. With stdout broken, the pubkey and log lines ended up inside the remote config-edit script and command line (reproduced; bash 5.2 purges the buffer). Fix: every message goes through say/warn, which on failure point the stream at /dev/null and flush the leftover there; remote command lines are built with printf -v %q instead of $(printf %q …).
• §10.1 passed when the node list could not be fetched (e.g. list HTTP 500 with detail 200 → hide ok), pre-existing. Hidden now requires detail 404 AND absent from the listing; a listing that cannot be fetched or grepped fails.
• grep errors are no longer read as "pubkey absent"; SIGHUP is handled like INT/TERM (129).

Review follow-up (41b2e0e3, 350f6dae, 877e0938, ec90ea24)

A later independent review of 5b2891ea found three problems. All are fixed test-first (the new tests gave 123 failures against 5b2891ea):

1. Existing blacklist rule removed. Teardown always removed TEST_NODE_PUBKEY, so a node that was already in nodeBlacklist lost its privacy rule. Now a read-only preflight (remote MODE=check, pubkey on ssh stdin) runs before any side effect and before the traps. It compares the way the server does: trimmed and lower-cased, as in buildBlacklistSet. If the pubkey is already listed in any case or padding, the run refuses with exit 2 and does not print the pubkey. Nothing is restarted, and the config bytes, mode and owner are unchanged. If the check cannot run (ssh down, config not JSON, a non-array list), the run fails with exit 1, also before any side effect. add now appends and remove drops only the exact canonical value, so the rest of the list is restored exactly: order, duplicates, other spellings and non-string entries. Before, jq unique sorted and de-duplicated the whole list and python de-duplicated it.

2. Vacuous topology pass. /api/topology does not exist, and the SPA fallback answered 200 HTML, which was reported clean. The check now:

   • uses /api/analytics/topology and waits out warm-up 503s for up to RESTART_WAIT_S;
   • requires HTTP 200 and exactly one JSON document with the TopologyResponse shape, with jq reading it via -n/[inputs];
   • compares the pubkey fields the server filters semantically: topRepeaters[].pubkey, topPairs[].pubkeyA/B, bestPathList[].pubkey, multiObsNodes[].pubkey and perObserverReach{}.rings[].nodes[].pubkey, extracted, lower-cased and matched whole-line against the canonical pubkey.

   HTML, a wrong shape, a wrong field type, non-JSON, an empty or blank body, two documents, 500, a 503 that never ends and curl failures all fail; nothing is skipped. Null arrays, which the server produces after filtering, are valid. jq is now required on the runner.

3. Case. TEST_NODE_PUBKEY is validated as before, then lower-cased once (tr reads it on stdin). The config entry, API paths, pattern files and the SQLite binding all use that canonical value. An end-to-end test with upper-case input against lower-case API and DB data passes, and removing the normalisation fails it.

The fake API no longer serves /api/topology; it serves the real route with the real response shape and can leak the pubkey into each part.

Env / path contract

Before	After
TARGET_DB_PATH (one path for container and host)	TARGET_CONTAINER_DB_PATH — path inside TARGET_CONTAINER, used only by docker exec … sqlite3
	TARGET_HOST_DB_PATH — path on TARGET_SSH_HOST, used only by the host sqlite3
ADMIN_API_TOKEN (dead API path)	removed — refuses to start
	TARGET_DB_PATH — removed — refuses to start, names the replacements
TEST_NODE_PUBKEY used as given	validated hex, then lower-cased once; that value is used everywhere
pubkey already in nodeBlacklist → removed by teardown	refused before any side effect (exit 2); check unreadable → exit 1
—	jq required on the runner (topology parsing)

Order: container runner (if its path is set and its sqlite3 passes the bind probe) → host runner (if its path is set). No path is ever tried in the other environment; a wrong path fails as retain-failed naming the variable. The only in-repo caller is the QA plan text, updated here. -readonly needs the WAL files of a running app; §10.2 runs after /api/stats has shown the restarted app is up (verified on a live WAL database below).

Signals / SIGPIPE

Exit = failures (0 = pass); 2 = refused to start (bad/removed settings, or the pubkey is already blacklisted); interrupted runs tear down and exit 130 (INT), 143 (TERM), 129 (HUP), 141 (PIPE), +1 if teardown failed. Without a PIPE handler bash still runs the EXIT trap but re-raises the signal, so the status was always 141 whatever teardown found, and teardown's next write to the dead stream cut the restore short. Inside teardown SIGPIPE is ignored (children inherit the ignore and see EPIPE). A shell started with SIGPIPE already ignored cannot trap it; it then runs to its normal status with dead streams sent to /dev/null.

argv evidence

• Unit (macOS + Linux): a fake target (fake ssh/docker/curl, every other command behind an argv-logging PATH shim, remote side included) runs the unmodified script end to end; on Linux each full run is additionally wrapped in strace -f -e execve (BLACKLIST_TEST_STRACE_DIR). At ec90ea24: 73 traced runs, 6673 execve, 0 containing the pubkey (either case), SQL or token.
• Full SSH/Docker (demo): 16 scenarios, strace -f -e execve on the runner and on the target's sshd: 1660 execve (379 runner, 1281 target); 0 hits for pubkey (lower/upper), SELECT, from_pubkey, token or /api/nodes URL; positive controls present (ssh 71, curl 82, grep 16, jq 27, sqlite3 9, docker 29). Remote edit argv is exactly bash -c "CFG=/srv/corescope-host/data/config.json MODE=add bash -s".

Verification

• bash -n both scripts; git diff --check; YAML parse of deploy.yml; go test -run 'ForkGuard|Workflow|ReleaseFastPath' in cmd/server (5 pass).
• bash qa/scripts/test-blacklist-sql.sh at ec90ea24: macOS bash 3.2 — 849 passed, 0 failed; Linux (ubuntu 24.04, bash 5.2, sqlite 3.45, jq 1.7) — 907 passed, 0 failed (incl. strace asserts); both again with SIGPIPE ignored on entry (849/0, 897/0). Branch base is 6334c427. Master has since moved to e51272d9 (PR test(server): replace distance lock timing threshold with a deterministic check #79 and PR feat(analytics): split advert relay airtime by route #86). The only overlap is deploy.yml: feat(analytics): split advert relay airtime by route #86 adds one JS test line and our two steps are untouched. The merge is clean, the YAML parses, TestForkGuard* pass on the merge result, and the suite passes on it (849/0). Neither PR touches the topology route, the blacklist or /api/nodes.
• shellcheck 0.9.0 -x: clean (isolated Linux container).
• CI finding (fixed in 5b2891ea): GitHub Actions starts steps with SIGPIPE ignored, which bash cannot trap, so the two kill -PIPE trap cases saw 0/1 instead of 141/142 — the script's documented ignored-on-entry behaviour, not a script fault. Those cases now reset SIGPIPE to default first (as the stream-breaker cases already did); the suite passes with SIGPIPE at default and ignored on entry, on macOS and Linux, and all 31 mutants are still killed with it ignored. Operational note: a runner that starts the script with SIGPIPE ignored gets the EPIPE path (run completes with its real status, dead streams go to /dev/null), not the 141 path.
• The new suite run against master's script fails across the new contract (argv, paths, removed vars, signals), as expected.
• Mutations: 54 mutants at ec90ea24, all killed on macOS; 52/54 on Linux. Added for the review follow-up (M32–M54): preflight skipped, moved after the traps, case-sensitive (jq, python3), no trim (jq, python3), printing the pubkey, exiting 0; unreadable config / non-list / parse error / missing list treated as "not blacklisted"; add sorting and de-duplicating again; topology on the old route, non-200 skipped, shape check removed, text grep instead of the field check, perObserverReach or pubkeyB not checked, case-sensitive compare, no 503 wait, empty body accepted; normalisation removed. The earlier set: They include pubkey back in ssh argv / remote jq argv / curl URL / grep argv, host runner using the container path and vice versa, legacy TARGET_DB_PATH fallback, sqlite allowed to create a file (both guards, and each guard alone), empty-file size check dropped, admin token accepted / admin API path reintroduced, SIGPIPE swallowed / reported as pass / untrapped, teardown not PIPE-safe, TERM → 0, teardown skipped, teardown failure not counted, SQL interpolated, query error → 0, grep error → absent, probe substring match, remote hex check dropped, hide OR-logic, list error → absent, HUP untrapped, HUP not handled inside teardown. The two Linux survivors (plain echo instead of say/warn; no buffer flush) only matter on bash 3.2, where they are killed — bash 5 has no replay bug.

Full isolated SSH/Docker QA (demo, own prefix)

App image built from this branch's Dockerfile (no sqlite3, as in production) plus a test variant with sqlite; --internal network, no published ports, outbound verified blocked; DISABLE_MOSQUITTO/DISABLE_CADDY; synthetic config, database and pubkeys. The script ran in a runner container over real OpenSSH to disposable sshd "target hosts" that drive the app via the Docker socket. The target sees the data at /srv/corescope-host/data, the app at /app/data — different container and host paths. Settings reached the runner through an env file, not argv.

#	Scenario	Exit
s01	container path set, container has no sqlite3 → host fallback with host path (success)	0
s02	container runner with container path; host path points at a decoy DB (1 row) — count 3 proves the container DB was read	0
s03	host path only	0
s04	wrong host path → refused before query, no file created	1
s05	wrong container path, sqlite-capable container → refused, no fallback, no file	1
s06	query error (DB without from_pubkey)	1
s07	no sqlite capability anywhere	1
s08/s09/s10	SIGINT / SIGTERM / SIGHUP mid-run	130 / 143 / 129
s11	TERM mid-run + INT during teardown	143
s12	stdout broken from start (SIGPIPE)	141
s13	only stderr broken + query error	1
s14	only stderr broken + TERM, then INT during teardown	143
s15/s16	legacy TARGET_DB_PATH / ADMIN_API_TOKEN → refuse before any side effect	2 / 2

After every run: config semantically identical with the same mode/owner, blacklist [], synthetic transmissions unchanged (digest), decoy/legacy DBs byte-identical, no new files in the data dirs, app running and the node back at /api/nodes/<pk> = 200, exit as expected. The -readonly open worked against the app's live WAL-mode database in both runners. #82's preserved resources were not touched.

Review follow-up on demo (same isolated setup, final script 877e0938/ec90ea24)

• The real routes, probed on the candidate app: /api/analytics/topology → 200 application/json with all TopologyResponse keys; /api/topology → 200 text/html (the SPA fallback that used to pass).

#	Scenario	Exit	Checked afterwards
f01 / f02	success, host fallback / container runner, real analytics topology	0 / 0	restored, node back
f03 / f04 / f05	pubkey already blacklisted: lower-case / upper-case / padded upper-case entry	2 / 2 / 2	config bytes identical, app not restarted, entry still present, pubkey not printed
f06	already blacklisted, upper-case TEST_NODE_PUBKEY	2	same as above
f07	upper-case TEST_NODE_PUBKEY, lower-case API and DB	0	canonical value everywhere, restored
f08	unsorted list with a duplicate and an upper-case entry	0	array restored exactly
f09	config without nodeBlacklist	0	comes back as [], otherwise identical

Earlier rounds on 41b2e0e3 covered the same plus SIGTERM, a dead stdout and a query error (all as expected). Across all 19 follow-up runs: 2252 execve (runner and target sshd), with 0 hits for the pubkey (either case), SELECT, from_pubkey, the token, /api/nodes or /api/analytics.

Independent review

A separate reviewer agent (not the author) checked quoting, stdin/file transport, argv evidence, the path contract, empty-file risk, signals/SIGPIPE/teardown, classification, mutation strength, #82's properties and scope: no blockers. Its should-fix items (the §10.1 list-failure pass; untested -readonly on WAL) and nits are addressed in 8a7bed26 / the demo run; its surviving mutants now have tests. A second pass on 8a7bed26 found no blockers and one test gap (HUP during teardown), closed in f97d4949; 5b2891ea fixes the CI-only SIGPIPE harness issue. Both are test-only; the script the demo exercised is unchanged.

For the review follow-up, a new independent reviewer checked the three fixes specifically: no blockers. Its should-fix items are fixed in 877e0938 and re-verified by the same reviewer (no blockers, all its mutants killed):

• an empty or blank topology body still passing;
• untested preflight error branches;
• no trimming like the server's.

Limitations

• The config edit rewrites config.json through jq/python (pretty-printed), as before; content, mode and owner are restored, bytes/formatting are not.
• Server finding, out of scope, not fixed here: filterBlacklistedFromTopology (cmd/server/routes.go) type-asserts typed slices such as []TopRepeater, but computeAnalyticsTopology builds []map[string]interface{}, so the server's topology blacklist filter does nothing on real data, and no server test covers it. On a target where a blacklisted node appears in topology, the corrected check will therefore report hide-failed, as it should. This is a production fix for a separate PR.
• On the demo's synthetic database the real topology has 0 pubkey fields (no path data), so the live runs prove the route, the status and the shape; leak detection in each field is proven by the unit tests.
• Observer IDs (observers[].id, the perObserverReach keys, observer_id) and hop prefixes are deliberately not asserted: the server does not filter observers, and hop prefixes would collide.
• A missing or null nodeBlacklist comes back as [] (the same to the server). There is a small race if an operator adds the same pubkey between the check and the add.
• The pubkey is still printed in the script's own log lines (stdout), as before — it is the node's public id; the contract here is argv.
• Remote argv on the app container side (docker exec … sqlite3 …) is evidenced through the docker CLI's argv on the target, not by tracing runc.
• -readonly needs the app's WAL files present (app running); with the app down §10.2 fails loudly (never a false pass).
• M20/M26 are only detectable on bash 3.2; CI (bash 5) cannot catch them.
• The SIGINT unit cases assume the suite is not started as a background job (SIGINT ignored) — that gives a false failure, never a false pass.

Workflow safety

The workflow diff is the renamed QA unit-test step and one added shellcheck step in the go-test job. No if: conditions, fork guards, GHCR, release, staging-deploy or badge steps change (TestForkGuard* pass). No staging, production or upstream system was contacted.

🤖 Generated with Claude Code

[dborup]

Review feedback addressed — head ec90ea24 (commits 41b2e0e3, 350f6dae, 877e0938, ec90ea24 on top of 5b2891ea; fast-forward, no rebase)

1. Existing blacklist state is preserved. A read-only preflight (remote MODE=check, pubkey on stdin, trimmed and case-insensitive like the server's buildBlacklistSet) runs before any side effect and before the traps.
   • Pubkey already blacklisted → exit 2. No restart, config bytes/mode/owner unchanged, the pubkey is not printed, and teardown never runs.
   • The check cannot read the config → exit 1, also before any change.
   • add appends and remove drops only the exact canonical value, so order, duplicates and other spellings in the list are restored exactly. Previously jq unique sorted and de-duplicated the list.
2. Topology check fixed. Uses /api/analytics/topology, waits out warm-up 503s, requires HTTP 200 and exactly one JSON document with the TopologyResponse shape, and compares the five pubkey-bearing parts the server filters, lower-cased and whole-line.
   • HTML (SPA), a wrong shape or type, non-JSON, empty or blank bodies, two documents, 500 and curl failures all fail; nothing is skipped.
   • The fake API no longer serves /api/topology.
3. Canonical pubkey. Validated as before, then lower-cased once and used for config, API paths, pattern files and the SQLite binding. An end-to-end test with upper-case input against lower-case API and DB data passes, and removing the normalisation fails it.

Verification

• Tests written first: 123 failures against 5b2891ea.
• Suite: macOS bash 3.2 849/0, Linux bash 5.2 907/0 with strace (73 runs, 6673 execve, 0 containing the pubkey, SQL or token). Both also pass with SIGPIPE ignored.
• ShellCheck -x is clean, git diff --check is clean, and the fork-guard/workflow tests pass.
• Mutations: 54 total; macOS 54/54 killed, Linux 52/54. The two Linux survivors only apply to bash 3.2.
• Demo, 19 isolated runs covering pre-blacklisted lower/upper/padded, upper-case input, an unsorted list, a missing list, the real analytics route, SIGTERM, a dead stdout and a query error: all as expected, with 0 argv hits in 2252 execve.
• A new independent reviewer found no blockers. Its should-fix items (empty topology body; untested preflight error branches; trimming) are fixed in 877e0938 and re-verified.

Out of scope, reported separately: the server's filterBlacklistedFromTopology is a no-op on real data. It type-asserts typed slices, but the store builds maps. On a real target the corrected check will therefore report hide-failed for a blacklisted node that appears in topology.

Master has moved to e51272d9 (#79, #86). The only overlap is deploy.yml, which merges cleanly, and the suite passes on the merge result.

🤖 Generated with Claude Code