From 684e1ab2b52ee4c613f1905f978b124247885e7a Mon Sep 17 00:00:00 2001 From: Openclaw Date: Wed, 23 Sep 2026 07:04:55 +0200 Subject: [PATCH 1/5] fix(qa): bind blacklist pubkey SQL parameter --- .github/workflows/deploy.yml | 3 + qa/scripts/blacklist-test.sh | 342 +++++++++++++++++++++---------- qa/scripts/test-blacklist-sql.sh | 135 ++++++++++++ 3 files changed, 369 insertions(+), 111 deletions(-) create mode 100755 qa/scripts/test-blacklist-sql.sh diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 505859c45..c0fac8264 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -91,6 +91,9 @@ jobs: - name: Staging disk-monitor unit tests (issue #1684) run: bash scripts/staging/test-disk-monitor.sh + - name: QA SQL parameter-binding unit tests (issue #1977) + run: bash qa/scripts/test-blacklist-sql.sh + - name: Lint CSS variables (issue #1128) run: | set -e diff --git a/qa/scripts/blacklist-test.sh b/qa/scripts/blacklist-test.sh index 94b5b4127..535f477f0 100755 --- a/qa/scripts/blacklist-test.sh +++ b/qa/scripts/blacklist-test.sh @@ -25,65 +25,24 @@ # ssh-failed → cannot reach/control target # restart-stuck → /api/stats not 200 within RESTART_WAIT_S # hide-failed → blacklisted pubkey still surfaced via API (§10.1 fail) -# retain-failed → blacklisted pubkey absent from DB (§10.2 fail) +# retain-failed → blacklisted pubkey absent from DB (§10.2 fail), or the +# §10.2 probe could not run at all — no sqlite3 on the target +# able to bind a parameter. The message names what is needed; +# there is no fallback to interpolated SQL. # teardown-failed→ post-test removal did not restore listing # # Exit code = number of failures (0 = pass). # PUBLIC repo: zero PII — no real pubkeys, IPs, or hostnames as defaults. +# +# Structure: helpers live at top level and the imperative body lives in main(), +# so test-blacklist-sql.sh can source this file and exercise individual helpers +# without running the suite. Same idiom as scripts/staging/disk-monitor.sh. set -uo pipefail -BASELINE_URL="${1:-}" -TARGET_URL="${2:-}" -if [[ -z "$BASELINE_URL" || -z "$TARGET_URL" ]]; then - echo "usage: $0 BASELINE_URL TARGET_URL (TEST_NODE_PUBKEY+TARGET_* via env)" >&2 - exit 2 -fi - -TEST_PUBKEY="${TEST_NODE_PUBKEY:-}" -TARGET_SSH_HOST="${TARGET_SSH_HOST:-}" -TARGET_SSH_KEY="${TARGET_SSH_KEY:-/root/.ssh/id_ed25519}" -TARGET_CONFIG_PATH="${TARGET_CONFIG_PATH:-}" -TARGET_CONTAINER="${TARGET_CONTAINER:-}" -TARGET_DB_PATH="${TARGET_DB_PATH:-}" -ADMIN_API_TOKEN="${ADMIN_API_TOKEN:-}" - -if [[ -z "$TEST_PUBKEY" || -z "$TARGET_SSH_HOST" || -z "$TARGET_CONFIG_PATH" || -z "$TARGET_CONTAINER" ]]; then - echo "error: TEST_NODE_PUBKEY, TARGET_SSH_HOST, TARGET_CONFIG_PATH, TARGET_CONTAINER are required" >&2 - exit 2 -fi - -# Hard input validation — these strings are interpolated into remote shell/SQL. -# Pubkey must be hex (MeshCore pubkeys are hex-encoded ed25519 prefixes). -if ! [[ "$TEST_PUBKEY" =~ ^[0-9a-fA-F]+$ ]]; then - echo "error: TEST_NODE_PUBKEY must be hex (got: redacted)" >&2 - exit 2 -fi -# Container name must match docker's allowed chars: [a-zA-Z0-9][a-zA-Z0-9_.-]* -if ! [[ "$TARGET_CONTAINER" =~ ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$ ]]; then - echo "error: TARGET_CONTAINER has illegal chars" >&2 - exit 2 -fi -# Config path must be an absolute, sane path (no spaces, quotes, $, ;, etc.). -if ! [[ "$TARGET_CONFIG_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then - echo "error: TARGET_CONFIG_PATH must be a sane absolute path" >&2 - exit 2 -fi -if [[ -n "$TARGET_DB_PATH" ]] && ! [[ "$TARGET_DB_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then - echo "error: TARGET_DB_PATH must be a sane absolute path" >&2 - exit 2 -fi - -CURL_TIMEOUT="${CURL_TIMEOUT:-60}" -RESTART_WAIT_S="${RESTART_WAIT_S:-120}" - -SSH_OPTS=(-i "$TARGET_SSH_KEY" -o StrictHostKeyChecking=accept-new -o ConnectTimeout=15 -o BatchMode=yes) +SSH_OPTS=() # populated by main() from TARGET_SSH_KEY ssh_t() { ssh "${SSH_OPTS[@]}" "$TARGET_SSH_HOST" "$@"; } -TMP=$(mktemp -d) -fails=0 -TEARDOWN_DONE=0 - # ----------------------------------------------------------------------------- # Teardown — MANDATORY in all exit paths. # ----------------------------------------------------------------------------- @@ -106,7 +65,6 @@ teardown() { rm -rf "$TMP" exit "$rc" } -trap teardown EXIT INT TERM # ----------------------------------------------------------------------------- # Helpers @@ -197,75 +155,237 @@ node_visible() { } # ----------------------------------------------------------------------------- -# §10.1 — hide +# §10.2 DB probe — bind the pubkey, do not interpolate it (issue #1977) # ----------------------------------------------------------------------------- -echo "=== §10.1 add $TEST_PUBKEY to nodeBlacklist ===" -if ! add_to_blacklist; then fails=$((fails+1)); exit "$fails"; fi -if ! restart_target; then fails=$((fails+1)); exit "$fails"; fi -if ! wait_for_stats; then fails=$((fails+1)); exit "$fails"; fi +# Batch flags, all in service of "the count is parseable and errors are visible": +# -bail stop at the first SQL error instead of running on +# -init /dev/null ignore the operator's ~/.sqliterc — a stray .mode there +# would make the count unparseable +# -noheader -list stdout is exactly the number, nothing else +SQLITE_ARGS=(-batch -bail -init /dev/null -noheader -list) +# Round-trip probe token. The value is arbitrary; it only has to come back intact. +SQLITE_PROBE_TOKEN="corescope-probe-ok" +SQLITE_RUNNER="" # "container" | "host", set by resolve_sqlite_runner +RETAIN_COUNT="" # set by read_retain_count -detail_code=$(fetch_code "$TARGET_URL/api/nodes/$TEST_PUBKEY" "$TMP/detail.json") -list_code=$(fetch_code "$TARGET_URL/api/nodes?limit=10000" "$TMP/list.json") -in_list=0 -if [[ "$list_code" == "200" ]] && grep -qF -- "\"$TEST_PUBKEY\"" "$TMP/list.json"; then - in_list=1 -fi -if [[ "$detail_code" == "404" || "$in_list" == "0" ]]; then - echo " ✅ hide ok: detail=$detail_code in_list=$in_list" -else - echo " ❌ hide-failed: detail=$detail_code in_list=$in_list — pubkey still surfaced" - fails=$((fails+1)) -fi +# Hex-encode a value for embedding in SQL as a blob literal. +# +# Why hex rather than quoting: the output alphabet is [0-9a-f], so no byte the +# caller passes can terminate a string literal or add a dot-command argument. +# That holds for arbitrary input, which is the point — the SQL layer stops +# depending on main()'s hex gate in order to be safe. +# +# `od -v` is load-bearing: without it od collapses runs of identical lines to +# '*' and long repetitive values encode wrongly. +sql_hex_literal() { + printf "x'%s'" "$(printf '%s' "$1" | od -An -v -tx1 | tr -d ' \n')" +} -topo_code=$(fetch_code "$TARGET_URL/api/topology" "$TMP/topo.json") -if [[ "$topo_code" != "200" ]]; then - echo " ⚠️ /api/topology HTTP $topo_code — skipping topology assertion" -elif grep -qF -- "$TEST_PUBKEY" "$TMP/topo.json"; then - echo " ❌ hide-failed: /api/topology references blacklisted pubkey" - fails=$((fails+1)) -else - echo " ✅ topology clean" -fi +# SQL for the §10.2 count, fed to sqlite3 on stdin. The SELECT text is a +# constant; the pubkey arrives as a bound parameter. +# +# Note the nested cast rather than `.parameter set :pubkey ''`: +# dot-command arguments are split on whitespace, so a value containing a space +# (e.g. "' OR 1=1 --") makes sqlite3 print the .parameter help to STDOUT, exit +# 0, and leave :pubkey unbound. COUNT(*) then returns 0 — which reads exactly +# like a passing security fix. -bail does not catch it either. +transmission_count_sql() { + printf '.parameter init\n' + printf '.parameter set :pubkey "cast(%s as text)"\n' "$(sql_hex_literal "$1")" + printf 'SELECT COUNT(*) FROM transmissions WHERE from_node = :pubkey;\n' +} -# ----------------------------------------------------------------------------- -# §10.2 — DB retain -# ----------------------------------------------------------------------------- -echo "=== §10.2 verify packets retained in DB ===" -count="" -if [[ -n "$ADMIN_API_TOKEN" ]]; then - # Read auth header from stdin so the token never enters argv (ps-safe). - code=$(printf 'header = "Authorization: Bearer %s"\n' "$ADMIN_API_TOKEN" | \ - curl -s -m "$CURL_TIMEOUT" -K - -o "$TMP/admin.json" -w "%{http_code}" \ - "$TARGET_URL/api/admin/transmissions?from_node=$TEST_PUBKEY&count=1" 2>/dev/null || echo "000") - if [[ "$code" == "200" ]]; then - count=$(jq -r '.count // ((.transmissions // []) | length)' "$TMP/admin.json" 2>/dev/null || echo "") +# Capability probe: bind a known value and read it back. A version number only +# implies that .parameter works; binding something and getting it back proves it +# on the binary actually in front of us, which is the operator's, not ours. +sqlite_probe_sql() { + printf '.parameter init\n' + printf '.parameter set :probe "cast(%s as text)"\n' "$(sql_hex_literal "$SQLITE_PROBE_TOKEN")" + printf 'SELECT :probe;\n' +} + +# Find a sqlite3 that can bind a parameter — in the container first, then on the +# host. Sets SQLITE_RUNNER; returns 1 if neither qualifies. There is deliberately +# no interpolating fallback: that would leave the vulnerable path in place under +# a nicer name. +# +# Probe stderr is collected rather than discarded, but only printed if BOTH +# probes fail. The container miss is the known-normal case — the app image has +# no sqlite3 (pure-Go driver, no CGO; Dockerfile:15) — so surfacing it on every +# run would be noise. +resolve_sqlite_runner() { + local probe out + probe=$(sqlite_probe_sql) + SQLITE_RUNNER="" + out=$(ssh_t "docker exec -i $(printf %q "$TARGET_CONTAINER") sqlite3 ${SQLITE_ARGS[*]} :memory:" \ + <<<"$probe" 2>>"$TMP/sqlite-probe.err") + if [[ "$out" == "$SQLITE_PROBE_TOKEN" ]]; then SQLITE_RUNNER="container"; return 0; fi + out=$(ssh_t "sqlite3 ${SQLITE_ARGS[*]} :memory:" <<<"$probe" 2>>"$TMP/sqlite-probe.err") + if [[ "$out" == "$SQLITE_PROBE_TOKEN" ]]; then SQLITE_RUNNER="host"; return 0; fi + return 1 +} + +# Run SQL from stdin against TARGET_DB_PATH via the resolved runner. Stderr is +# left alone so the caller can capture it, and the exit status is sqlite3's. +# The SQL crosses on stdin, so only the container name and db path still need +# printf %q for the remote shell. docker exec needs -i to attach stdin. +run_sqlite() { + case "$SQLITE_RUNNER" in + container) ssh_t "docker exec -i $(printf %q "$TARGET_CONTAINER") sqlite3 ${SQLITE_ARGS[*]} $(printf %q "$TARGET_DB_PATH")" ;; + host) ssh_t "sqlite3 ${SQLITE_ARGS[*]} $(printf %q "$TARGET_DB_PATH")" ;; + *) echo "run_sqlite: no runner resolved" >&2; return 127 ;; + esac +} + +# Read the retained-transmission count into RETAIN_COUNT. Prints a classified +# "retain-failed" line and returns 1 on failure, so §10.2 has exactly one place +# that increments $fails. +read_retain_count() { + RETAIN_COUNT="" + local code + if [[ -n "$ADMIN_API_TOKEN" ]]; then + # Read auth header from stdin so the token never enters argv (ps-safe). + code=$(printf 'header = "Authorization: Bearer %s"\n' "$ADMIN_API_TOKEN" | \ + curl -s -m "$CURL_TIMEOUT" -K - -o "$TMP/admin.json" -w "%{http_code}" \ + "$TARGET_URL/api/admin/transmissions?from_node=$TEST_PUBKEY&count=1" 2>/dev/null || echo "000") + if [[ "$code" == "200" ]]; then + RETAIN_COUNT=$(jq -r '.count // ((.transmissions // []) | length)' "$TMP/admin.json" 2>/dev/null || echo "") + fi + if [[ -n "$RETAIN_COUNT" ]]; then return 0; fi fi -fi -if [[ -z "$count" ]]; then + if [[ -z "$TARGET_DB_PATH" ]]; then echo " ❌ retain-failed: TARGET_DB_PATH unset and no ADMIN_API_TOKEN — cannot probe" + return 1 + fi + if ! resolve_sqlite_runner; then + echo " ❌ retain-failed: no sqlite3 able to bind a parameter on the target" + echo " tried: docker exec -i $TARGET_CONTAINER sqlite3, then sqlite3 on $TARGET_SSH_HOST" + echo " need: the sqlite3 CLI reachable over ssh, supporting '.parameter set'" + cat "$TMP/sqlite-probe.err" >&2 + return 1 + fi + echo " sqlite3 runner: $SQLITE_RUNNER" + if ! RETAIN_COUNT=$(run_sqlite <<<"$(transmission_count_sql "$TEST_PUBKEY")" 2>"$TMP/sqlite.err"); then + echo " ❌ retain-failed: sqlite3 query failed via $SQLITE_RUNNER" + cat "$TMP/sqlite.err" >&2 + RETAIN_COUNT="" + return 1 + fi + return 0 +} + +# ----------------------------------------------------------------------------- +# main +# ----------------------------------------------------------------------------- +main() { + BASELINE_URL="${1:-}" + TARGET_URL="${2:-}" + if [[ -z "$BASELINE_URL" || -z "$TARGET_URL" ]]; then + echo "usage: $0 BASELINE_URL TARGET_URL (TEST_NODE_PUBKEY+TARGET_* via env)" >&2 + exit 2 + fi + + TEST_PUBKEY="${TEST_NODE_PUBKEY:-}" + TARGET_SSH_HOST="${TARGET_SSH_HOST:-}" + TARGET_SSH_KEY="${TARGET_SSH_KEY:-/root/.ssh/id_ed25519}" + TARGET_CONFIG_PATH="${TARGET_CONFIG_PATH:-}" + TARGET_CONTAINER="${TARGET_CONTAINER:-}" + TARGET_DB_PATH="${TARGET_DB_PATH:-}" + ADMIN_API_TOKEN="${ADMIN_API_TOKEN:-}" + + if [[ -z "$TEST_PUBKEY" || -z "$TARGET_SSH_HOST" || -z "$TARGET_CONFIG_PATH" || -z "$TARGET_CONTAINER" ]]; then + echo "error: TEST_NODE_PUBKEY, TARGET_SSH_HOST, TARGET_CONFIG_PATH, TARGET_CONTAINER are required" >&2 + exit 2 + fi + + # Hard input validation — these strings are interpolated into the remote shell. + # §10.2's SQL binds TEST_PUBKEY as a parameter rather than interpolating it, so + # for the SQL layer this gate is defence in depth rather than the only guard + # (issue #1977). Keep it: redundant is not the same as wrong. + # Pubkey must be hex (MeshCore pubkeys are hex-encoded ed25519 prefixes). + if ! [[ "$TEST_PUBKEY" =~ ^[0-9a-fA-F]+$ ]]; then + echo "error: TEST_NODE_PUBKEY must be hex (got: redacted)" >&2 + exit 2 + fi + # Container name must match docker's allowed chars: [a-zA-Z0-9][a-zA-Z0-9_.-]* + if ! [[ "$TARGET_CONTAINER" =~ ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$ ]]; then + echo "error: TARGET_CONTAINER has illegal chars" >&2 + exit 2 + fi + # Config path must be an absolute, sane path (no spaces, quotes, $, ;, etc.). + if ! [[ "$TARGET_CONFIG_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then + echo "error: TARGET_CONFIG_PATH must be a sane absolute path" >&2 + exit 2 + fi + if [[ -n "$TARGET_DB_PATH" ]] && ! [[ "$TARGET_DB_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then + echo "error: TARGET_DB_PATH must be a sane absolute path" >&2 + exit 2 + fi + + CURL_TIMEOUT="${CURL_TIMEOUT:-60}" + RESTART_WAIT_S="${RESTART_WAIT_S:-120}" + + SSH_OPTS=(-i "$TARGET_SSH_KEY" -o StrictHostKeyChecking=accept-new -o ConnectTimeout=15 -o BatchMode=yes) + + TMP=$(mktemp -d) + fails=0 + TEARDOWN_DONE=0 + trap teardown EXIT INT TERM + + # --------------------------------------------------------------------------- + # §10.1 — hide + # --------------------------------------------------------------------------- + echo "=== §10.1 add $TEST_PUBKEY to nodeBlacklist ===" + if ! add_to_blacklist; then fails=$((fails+1)); exit "$fails"; fi + if ! restart_target; then fails=$((fails+1)); exit "$fails"; fi + if ! wait_for_stats; then fails=$((fails+1)); exit "$fails"; fi + + detail_code=$(fetch_code "$TARGET_URL/api/nodes/$TEST_PUBKEY" "$TMP/detail.json") + list_code=$(fetch_code "$TARGET_URL/api/nodes?limit=10000" "$TMP/list.json") + in_list=0 + if [[ "$list_code" == "200" ]] && grep -qF -- "\"$TEST_PUBKEY\"" "$TMP/list.json"; then + in_list=1 + fi + if [[ "$detail_code" == "404" || "$in_list" == "0" ]]; then + echo " ✅ hide ok: detail=$detail_code in_list=$in_list" + else + echo " ❌ hide-failed: detail=$detail_code in_list=$in_list — pubkey still surfaced" + fails=$((fails+1)) + fi + + topo_code=$(fetch_code "$TARGET_URL/api/topology" "$TMP/topo.json") + if [[ "$topo_code" != "200" ]]; then + echo " ⚠️ /api/topology HTTP $topo_code — skipping topology assertion" + elif grep -qF -- "$TEST_PUBKEY" "$TMP/topo.json"; then + echo " ❌ hide-failed: /api/topology references blacklisted pubkey" fails=$((fails+1)) else - # TEST_PUBKEY is hex-validated → safe to inline single-quoted in SQL. - # Container/db path also validated; printf %q for defense in depth. - q="SELECT COUNT(*) FROM transmissions WHERE from_node = '$TEST_PUBKEY';" - qq=$(printf %q "$q") - if ! count=$(ssh_t "docker exec $(printf %q "$TARGET_CONTAINER") sqlite3 $(printf %q "$TARGET_DB_PATH") $qq" 2>/dev/null); then - count=$(ssh_t "sqlite3 $(printf %q "$TARGET_DB_PATH") $qq" 2>/dev/null || echo "") - fi + echo " ✅ topology clean" fi -fi -if [[ -z "$count" ]]; then - echo " ❌ retain-failed: could not read transmissions count" - fails=$((fails+1)) -elif [[ "$count" =~ ^[0-9]+$ ]] && (( count > 0 )); then - echo " ✅ DB retains $count packets from $TEST_PUBKEY" -else - echo " ❌ retain-failed: count=$count (expected > 0)" - fails=$((fails+1)) -fi + # --------------------------------------------------------------------------- + # §10.2 — DB retain + # --------------------------------------------------------------------------- + echo "=== §10.2 verify packets retained in DB ===" + if ! read_retain_count; then + # read_retain_count already printed the classified reason. Counting here and + # nowhere else: the old code incremented $fails for the "TARGET_DB_PATH + # unset" case and then again for the empty count it left behind. + fails=$((fails+1)) + elif [[ "$RETAIN_COUNT" =~ ^[0-9]+$ ]] && (( RETAIN_COUNT > 0 )); then + echo " ✅ DB retains $RETAIN_COUNT packets from $TEST_PUBKEY" + else + echo " ❌ retain-failed: count=$RETAIN_COUNT (expected > 0)" + fails=$((fails+1)) + fi + + echo "=== summary: $fails failure(s) before teardown ===" + # trap handles teardown + exit + exit "$fails" +} -echo "=== summary: $fails failure(s) before teardown ===" -# trap handles teardown + exit -exit "$fails" +# Only run main when executed directly (not when sourced by tests). +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + main "$@" +fi diff --git a/qa/scripts/test-blacklist-sql.sh b/qa/scripts/test-blacklist-sql.sh new file mode 100755 index 000000000..770d6e065 --- /dev/null +++ b/qa/scripts/test-blacklist-sql.sh @@ -0,0 +1,135 @@ +#!/usr/bin/env bash +# test-blacklist-sql.sh — unit tests for the §10.2 SQL construction in +# qa/scripts/blacklist-test.sh (issue #1977). Sources the script and exercises +# its pure helpers, plus a real local sqlite3 against a throwaway fixture DB. +# +# Run: bash qa/scripts/test-blacklist-sql.sh +# Exits non-zero if any case fails. +# +# The point of the sqlite3 group is that BOTH directions are asserted. A test +# that only checks "the injection payload returns 0" passes just as happily when +# the query is silently broken and returns 0 for everything, so the legitimate +# pubkey must be shown to still return the row it should. + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +# shellcheck source=blacklist-test.sh +. "$SCRIPT_DIR/blacklist-test.sh" + +PASS=0 +FAIL=0 + +assert_eq() { + local label="$1" expected="$2" actual="$3" + if [ "$expected" = "$actual" ]; then + PASS=$((PASS + 1)) + else + FAIL=$((FAIL + 1)) + echo "FAIL: $label — expected '$expected' got '$actual'" >&2 + fi +} + +assert_match() { + local label="$1" pattern="$2" actual="$3" + if [[ "$actual" =~ $pattern ]]; then + PASS=$((PASS + 1)) + else + FAIL=$((FAIL + 1)) + echo "FAIL: $label — '$actual' does not match /$pattern/" >&2 + fi +} + +# ----- sql_hex_literal ------------------------------------------------------ +# The security property: whatever goes in, the SQL text it produces is drawn +# from [0-9a-f] only. No caller-supplied byte can close a string literal or add +# a dot-command argument. Needs no sqlite3, so this group always runs. +assert_eq "hex of deadbeef" "x'6465616462656566'" "$(sql_hex_literal deadbeef)" +assert_eq "hex of empty" "x''" "$(sql_hex_literal "")" + +HEX_ONLY="^x'[0-9a-f]*'\$" +assert_match "alphabet: sql quote payload" "$HEX_ONLY" "$(sql_hex_literal "' OR 1=1 --")" +assert_match "alphabet: drop table" "$HEX_ONLY" "$(sql_hex_literal '"; DROP TABLE transmissions; --')" +assert_match "alphabet: backslash" "$HEX_ONLY" "$(sql_hex_literal 'a\b')" +assert_match "alphabet: dollar and backtick" "$HEX_ONLY" "$(sql_hex_literal '$(id) `id`')" +assert_match "alphabet: embedded newline" "$HEX_ONLY" "$(sql_hex_literal "$(printf 'a\nb')")" +assert_match "alphabet: multibyte" "$HEX_ONLY" "$(sql_hex_literal 'héllo')" + +# `od` without -v collapses runs of identical lines to '*'. A long repetitive +# value is the case that catches losing the flag. +LONG=$(printf 'x%.0s' $(seq 1 4096)) +LONG_HEX=$(sql_hex_literal "$LONG") +assert_match "alphabet: 4096 repeated bytes" "$HEX_ONLY" "$LONG_HEX" +# 4096 bytes → 8192 hex digits, plus the 3 chars of x''. A collapsed run would +# be far shorter and would also fail the alphabet check on '*'. +assert_eq "no od line-collapse in 4096-byte value" "8192" "$(( ${#LONG_HEX} - 3 ))" + +# ----- against a real sqlite3 ---------------------------------------------- +if ! command -v sqlite3 >/dev/null 2>&1; then + echo "SKIP: sqlite3 not on PATH — skipping the ${#SQLITE_ARGS[@]}-flag query group" >&2 + echo " (the alphabet assertions above still ran)" >&2 +else + FIXTURE_DIR=$(mktemp -d) + trap 'rm -rf "$FIXTURE_DIR"' EXIT + DB="$FIXTURE_DIR/fixture.db" + EMPTY_DB="$FIXTURE_DIR/no-table.db" + sqlite3 "$DB" \ + "CREATE TABLE transmissions(from_node TEXT); INSERT INTO transmissions VALUES('deadbeef'),('cafebabe');" + sqlite3 "$EMPTY_DB" "CREATE TABLE unrelated(x);" + + run_local() { sqlite3 "${SQLITE_ARGS[@]}" "$1"; } + + # The capability probe must round-trip on this machine, or the assertions + # below would be testing nothing. + assert_eq "probe round-trips" "$SQLITE_PROBE_TOKEN" "$(sqlite_probe_sql | run_local :memory:)" + + # POSITIVE CONTROL: a legitimate pubkey still returns its row. Without this, + # a silently broken query looks like a passing security fix. + out=$(transmission_count_sql deadbeef | run_local "$DB"); rc=$? + assert_eq "legit pubkey → its row" "1" "$out" + assert_eq "legit pubkey → exit 0" "0" "$rc" + assert_eq "other legit pubkey" "1" "$(transmission_count_sql cafebabe | run_local "$DB")" + assert_eq "absent pubkey → 0" "0" "$(transmission_count_sql abc123 | run_local "$DB")" + + # NEGATIVE: the payload binds as a literal that matches nothing. The table + # holds 2 rows, so a structural injection would return 2, not 0. + out=$(transmission_count_sql "' OR 1=1 --" | run_local "$DB"); rc=$? + assert_eq "injection payload → 0 rows" "0" "$out" + assert_eq "injection payload → exit 0" "0" "$rc" + assert_eq "table really does hold 2 rows" "2" \ + "$(run_local "$DB" <<<'SELECT COUNT(*) FROM transmissions;')" + + # Interpolating the same payload the old way returns the whole table. This is + # the behaviour the change removes; asserting it keeps the test honest about + # what "0" above is worth. + legacy="SELECT COUNT(*) FROM transmissions WHERE from_node = '' OR 1=1 --';" + assert_eq "old interpolated form leaked the table" "2" "$(run_local "$DB" <<<"$legacy")" + + # Multibyte and whitespace values bind as themselves rather than erroring. + sqlite3 "$DB" "INSERT INTO transmissions VALUES('héllo wörld');" + assert_eq "multibyte value with a space binds" "1" \ + "$(transmission_count_sql 'héllo wörld' | run_local "$DB")" + + # ERROR SURFACING: a broken query must be distinguishable from an empty + # result — non-zero exit and something on stderr, not a silent "". + err_file="$FIXTURE_DIR/err" + out=$(transmission_count_sql deadbeef | run_local "$EMPTY_DB" 2>"$err_file"); rc=$? + if [ "$rc" -ne 0 ]; then PASS=$((PASS + 1)); else + FAIL=$((FAIL + 1)); echo "FAIL: missing table — expected non-zero exit, got $rc" >&2 + fi + if [ -s "$err_file" ]; then PASS=$((PASS + 1)); else + FAIL=$((FAIL + 1)); echo "FAIL: missing table — expected a message on stderr" >&2 + fi + assert_eq "missing table → no count on stdout" "" "$out" + + # run_sqlite with no resolved runner must refuse rather than guess. + SQLITE_RUNNER="" + if run_sqlite /dev/null 2>&1; then + FAIL=$((FAIL + 1)); echo "FAIL: run_sqlite with no runner — expected non-zero exit" >&2 + else + PASS=$((PASS + 1)) + fi +fi + +echo "test-blacklist-sql.sh: $PASS passed, $FAIL failed" +[ "$FAIL" -eq 0 ] From 5ec3873cd1b81034b52070744ecd259e2facf496 Mon Sep 17 00:00:00 2001 From: Openclaw Date: Wed, 23 Sep 2026 14:17:18 +0200 Subject: [PATCH 2/5] fix(qa): count retained transmissions by from_pubkey, not from_node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The §10.2 probe in blacklist-test.sh counted `transmissions WHERE from_node = :pubkey`, but no CoreScope database has a from_node column: the ingestor's CREATE TABLE, the from_pubkey_v1 migration and internal/dbschema's AssertReady all define transmissions.from_pubkey. Against a real target the probe could only fail with "no such column". The unit test hid this by building its own table with an invented from_node column. It now takes the transmissions DDL straight from cmd/ingestor/db.go, also runs the query against the committed staging-captured fixture, and keeps the old from_node table only as a negative case that must error rather than count 0. from_pubkey is written only for ADVERTs, as hex.EncodeToString output (lowercase). The hex gate and the server's nodeBlacklist accept any case, so the bound value is lowercased in SQL; binding, stdin transport, the capability probe and the container->host fallback are unchanged. New coverage: exact-match and case handling, prefix non-match, more injection payloads, empty/whitespace/multibyte/10k values, missing table/column surfacing, and a stubbed ssh_t that runs the remote command through a real bash -c to prove the fallback, the rejection of a sqlite3 that cannot bind, and that SQL and pubkey travel on stdin, never argv. Co-Authored-By: Claude Opus 5.5 --- qa/scripts/blacklist-test.sh | 12 +- qa/scripts/test-blacklist-sql.sh | 220 ++++++++++++++++++++++++++----- 2 files changed, 196 insertions(+), 36 deletions(-) diff --git a/qa/scripts/blacklist-test.sh b/qa/scripts/blacklist-test.sh index 535f477f0..205ff8f7e 100755 --- a/qa/scripts/blacklist-test.sh +++ b/qa/scripts/blacklist-test.sh @@ -25,7 +25,8 @@ # ssh-failed → cannot reach/control target # restart-stuck → /api/stats not 200 within RESTART_WAIT_S # hide-failed → blacklisted pubkey still surfaced via API (§10.1 fail) -# retain-failed → blacklisted pubkey absent from DB (§10.2 fail), or the +# retain-failed → no transmissions.from_pubkey rows (ADVERTs) for the +# blacklisted pubkey in the DB (§10.2 fail), or the # §10.2 probe could not run at all — no sqlite3 on the target # able to bind a parameter. The message names what is needed; # there is no fallback to interpolated SQL. @@ -189,10 +190,17 @@ sql_hex_literal() { # (e.g. "' OR 1=1 --") makes sqlite3 print the .parameter help to STDOUT, exit # 0, and leave :pubkey unbound. COUNT(*) then returns 0 — which reads exactly # like a passing security fix. -bail does not catch it either. +# +# The column is transmissions.from_pubkey (cmd/ingestor/db.go CREATE TABLE and +# the from_pubkey_v1 migration; asserted by internal/dbschema). The ingestor +# fills it only for ADVERTs, with hex.EncodeToString output — lowercase. The +# script's hex gate and the server's nodeBlacklist both accept any case, so the +# bound value is lowercased inside SQL; the parameter itself is still bound. +# There is no from_node column: querying it errors on every real database. transmission_count_sql() { printf '.parameter init\n' printf '.parameter set :pubkey "cast(%s as text)"\n' "$(sql_hex_literal "$1")" - printf 'SELECT COUNT(*) FROM transmissions WHERE from_node = :pubkey;\n' + printf 'SELECT COUNT(*) FROM transmissions WHERE from_pubkey = lower(:pubkey);\n' } # Capability probe: bind a known value and read it back. A version number only diff --git a/qa/scripts/test-blacklist-sql.sh b/qa/scripts/test-blacklist-sql.sh index 770d6e065..4dfdc8b4b 100755 --- a/qa/scripts/test-blacklist-sql.sh +++ b/qa/scripts/test-blacklist-sql.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # test-blacklist-sql.sh — unit tests for the §10.2 SQL construction in # qa/scripts/blacklist-test.sh (issue #1977). Sources the script and exercises -# its pure helpers, plus a real local sqlite3 against a throwaway fixture DB. +# its pure helpers, plus a real local sqlite3 against throwaway fixture DBs. # # Run: bash qa/scripts/test-blacklist-sql.sh # Exits non-zero if any case fails. @@ -10,10 +10,19 @@ # that only checks "the injection payload returns 0" passes just as happily when # the query is silently broken and returns 0 for everything, so the legitimate # pubkey must be shown to still return the row it should. +# +# The fixture schema is NOT hand-written: it is the transmissions CREATE TABLE +# extracted from cmd/ingestor/db.go, and the query is also run against the +# committed staging-captured test-fixtures/e2e-fixture.db. An earlier version +# invented a `from_node` column that no CoreScope database has, and so passed +# while the real probe could never succeed. set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" +INGESTOR_DB_GO="$REPO_ROOT/cmd/ingestor/db.go" +REAL_FIXTURE="$REPO_ROOT/test-fixtures/e2e-fixture.db" # shellcheck source=blacklist-test.sh . "$SCRIPT_DIR/blacklist-test.sh" @@ -40,6 +49,16 @@ assert_match() { fi } +assert_true() { + local label="$1"; shift + if "$@"; then PASS=$((PASS + 1)); else + FAIL=$((FAIL + 1)); echo "FAIL: $label" >&2 + fi +} + +contains() { [[ "$1" == *"$2"* ]]; } +lacks() { [[ "$1" != *"$2"* ]]; } + # ----- sql_hex_literal ------------------------------------------------------ # The security property: whatever goes in, the SQL text it produces is drawn # from [0-9a-f] only. No caller-supplied byte can close a string literal or add @@ -64,63 +83,196 @@ assert_match "alphabet: 4096 repeated bytes" "$HEX_ONLY" "$LONG_HEX" # be far shorter and would also fail the alphabet check on '*'. assert_eq "no od line-collapse in 4096-byte value" "8192" "$(( ${#LONG_HEX} - 3 ))" +# ----- query text ------------------------------------------------------------- +# Synthetic 32-byte pubkeys in the ingestor's form (hex.EncodeToString → lowercase). +PK_A="0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +PK_B="fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210" +PK_ABSENT="00000000000000000000000000000000000000000000000000000000000000ff" +PK_A_UPPER=$(printf '%s' "$PK_A" | tr 'a-f' 'A-F') + +COUNT_SQL=$(transmission_count_sql "$PK_A") +assert_true "query targets transmissions.from_pubkey" contains "$COUNT_SQL" "WHERE from_pubkey = lower(:pubkey)" +assert_true "query does not reference from_node" lacks "$COUNT_SQL" "from_node" +assert_true "query text does not carry the raw pubkey" lacks "$COUNT_SQL" "$PK_A" + +# ----- schema taken from the ingestor ------------------------------------------ +# Pull the transmissions DDL out of cmd/ingestor/db.go instead of restating it. +transmissions_ddl() { + awk '/CREATE TABLE IF NOT EXISTS transmissions \(/{p=1} p{print} p&&/^[[:space:]]*\);/{exit}' "$INGESTOR_DB_GO" +} +DDL=$(transmissions_ddl) +assert_match "ingestor DDL extracted" "CREATE TABLE IF NOT EXISTS transmissions \\(" "$DDL" +assert_match "ingestor DDL has from_pubkey" "from_pubkey[[:space:]]+TEXT" "$DDL" +assert_true "ingestor DDL has no from_node" lacks "$DDL" "from_node" + # ----- against a real sqlite3 ---------------------------------------------- if ! command -v sqlite3 >/dev/null 2>&1; then echo "SKIP: sqlite3 not on PATH — skipping the ${#SQLITE_ARGS[@]}-flag query group" >&2 - echo " (the alphabet assertions above still ran)" >&2 + echo " (the alphabet, query-text and schema assertions above still ran)" >&2 else FIXTURE_DIR=$(mktemp -d) trap 'rm -rf "$FIXTURE_DIR"' EXIT DB="$FIXTURE_DIR/fixture.db" + LEGACY_DB="$FIXTURE_DIR/legacy-from-node.db" EMPTY_DB="$FIXTURE_DIR/no-table.db" - sqlite3 "$DB" \ - "CREATE TABLE transmissions(from_node TEXT); INSERT INTO transmissions VALUES('deadbeef'),('cafebabe');" + + # Schema-realistic fixture: the ingestor's own DDL. Three ADVERTs from A, one + # from B, and two non-ADVERT rows whose from_pubkey is NULL (the ingestor only + # attributes ADVERTs), so a structural injection has 6 rows to leak. + printf '%s\n' "$DDL" | sqlite3 "$DB" + sqlite3 "$DB" <&1); rc=$? + assert_eq "payload binds literally: $(printf %q "$payload")" "0" "$out" + assert_eq "payload exit 0: $(printf %q "$payload")" "0" "$rc" + done # Interpolating the same payload the old way returns the whole table. This is # the behaviour the change removes; asserting it keeps the test honest about # what "0" above is worth. - legacy="SELECT COUNT(*) FROM transmissions WHERE from_node = '' OR 1=1 --';" - assert_eq "old interpolated form leaked the table" "2" "$(run_local "$DB" <<<"$legacy")" + legacy="SELECT COUNT(*) FROM transmissions WHERE from_pubkey = '' OR 1=1 --';" + assert_eq "interpolated form leaks the table" "6" "$(run_local "$DB" <<<"$legacy")" - # Multibyte and whitespace values bind as themselves rather than erroring. - sqlite3 "$DB" "INSERT INTO transmissions VALUES('héllo wörld');" - assert_eq "multibyte value with a space binds" "1" \ - "$(transmission_count_sql 'héllo wörld' | run_local "$DB")" + # Multibyte, whitespace, empty and long values bind as themselves. + LONG_PK=$(printf 'ab%.0s' $(seq 1 5000)) + sqlite3 "$DB" "INSERT INTO transmissions(raw_hex,hash,first_seen,from_pubkey) VALUES + ('00','m1','t','héllo wörld'),('00','m2','t',''),('00','m3','t',' '),('00','m4','t','$LONG_PK');" + assert_eq "multibyte value with a space binds" "1" "$(count 'héllo wörld' "$DB")" + assert_eq "empty value binds as ''" "1" "$(count '' "$DB")" + assert_eq "whitespace value binds as itself" "1" "$(count ' ' "$DB")" + assert_eq "10000-byte value binds" "1" "$(count "$LONG_PK" "$DB")" - # ERROR SURFACING: a broken query must be distinguishable from an empty - # result — non-zero exit and something on stderr, not a silent "". - err_file="$FIXTURE_DIR/err" - out=$(transmission_count_sql deadbeef | run_local "$EMPTY_DB" 2>"$err_file"); rc=$? - if [ "$rc" -ne 0 ]; then PASS=$((PASS + 1)); else - FAIL=$((FAIL + 1)); echo "FAIL: missing table — expected non-zero exit, got $rc" >&2 - fi - if [ -s "$err_file" ]; then PASS=$((PASS + 1)); else - FAIL=$((FAIL + 1)); echo "FAIL: missing table — expected a message on stderr" >&2 + # REAL DATA: the committed staging-captured fixture. Its most-attributed + # pubkey, counted by the script's query, must equal a direct count. + if [ -f "$REAL_FIXTURE" ]; then + cp "$REAL_FIXTURE" "$FIXTURE_DIR/real.db" + real_pk=$(run_local "$FIXTURE_DIR/real.db" <<<"SELECT from_pubkey FROM transmissions WHERE from_pubkey IS NOT NULL GROUP BY from_pubkey ORDER BY COUNT(*) DESC, from_pubkey LIMIT 1;") + real_n=$(run_local "$FIXTURE_DIR/real.db" <<<"SELECT COUNT(*) FROM transmissions WHERE from_pubkey = '$real_pk';") + assert_match "real fixture has an attributed pubkey" '^[0-9a-f]{64}$' "$real_pk" + assert_match "real fixture count > 0" '^[1-9][0-9]*$' "$real_n" + assert_eq "script query on real fixture" "$real_n" "$(count "$real_pk" "$FIXTURE_DIR/real.db")" + else + FAIL=$((FAIL + 1)); echo "FAIL: $REAL_FIXTURE missing" >&2 fi - assert_eq "missing table → no count on stdout" "" "$out" + + # ERROR SURFACING: a broken query must be distinguishable from an empty + # result — non-zero exit and something on stderr, not a silent "" or "0". + for bad in "$LEGACY_DB:no such column: from_pubkey" "$EMPTY_DB:no such table: transmissions"; do + bad_db=${bad%%:*}; want=${bad#*:} + err_file="$FIXTURE_DIR/err" + out=$(count "$PK_A" "$bad_db" 2>"$err_file"); rc=$? + assert_true "$want → non-zero exit (got $rc)" test "$rc" -ne 0 + assert_true "$want → named on stderr" contains "$(cat "$err_file")" "$want" + assert_eq "$want → no count on stdout" "" "$out" + done + + # ----- runner selection and transport, with ssh stubbed --------------------- + # ssh_t is replaced by a stub that logs its argv, captures stdin, and runs the + # remote command string through a real `bash -c`, so the script's own quoting + # is what gets parsed. "docker exec -i NAME" is peeled off when the stub + # container is said to have sqlite3; otherwise it fails like the app image. + STUB_ARGV="$FIXTURE_DIR/ssh.argv"; STUB_STDIN="$FIXTURE_DIR/ssh.stdin" + STUB_CONTAINER_SQLITE=0 + STUB_PATH="$PATH" + ssh_t() { + local cmd="$1" in + printf '%s\n' "$*" >>"$STUB_ARGV" + in=$(cat); printf '%s\n' "$in" >>"$STUB_STDIN" + case "$cmd" in + "docker exec -i "*) + if [ "$STUB_CONTAINER_SQLITE" != 1 ]; then + echo 'exec: "sqlite3": executable file not found in $PATH' >&2; return 126 + fi + cmd=${cmd#docker exec -i }; cmd=${cmd#* } ;; + esac + printf '%s\n' "$in" | PATH="$STUB_PATH" bash -c "$cmd" + } + reset_stub() { : >"$STUB_ARGV"; : >"$STUB_STDIN"; } + TMP="$FIXTURE_DIR"; ADMIN_API_TOKEN=""; TARGET_CONTAINER="corescope-stub" + TARGET_SSH_HOST="stub-host"; TARGET_DB_PATH="$DB"; TEST_PUBKEY="$PK_A" + + # Container has no sqlite3 → host runner, correct count. + reset_stub + read_retain_count >/dev/null 2>&1; rc=$? + assert_eq "fallback: rc" "0" "$rc" + assert_eq "fallback: runner" "host" "$SQLITE_RUNNER" + assert_eq "fallback: count" "3" "$RETAIN_COUNT" + argv=$(cat "$STUB_ARGV"); stdin=$(cat "$STUB_STDIN") + assert_true "transport: container tried first" contains "$(head -1 "$STUB_ARGV")" "docker exec -i corescope-stub sqlite3" + assert_true "transport: pubkey not in remote argv" lacks "$argv" "$PK_A" + assert_true "transport: SQL not in remote argv" lacks "$argv" "SELECT" + assert_true "transport: column not in remote argv" lacks "$argv" "from_pubkey" + assert_true "transport: SQL arrives on stdin" contains "$stdin" "SELECT COUNT(*) FROM transmissions WHERE from_pubkey = lower(:pubkey);" + assert_true "transport: pubkey arrives hex-encoded on stdin" contains "$stdin" "$(sql_hex_literal "$PK_A")" + assert_true "transport: raw pubkey never on stdin" lacks "$stdin" "$PK_A" + + # Container has sqlite3 → container runner. + reset_stub; STUB_CONTAINER_SQLITE=1 + read_retain_count >/dev/null 2>&1; rc=$? + assert_eq "container: rc" "0" "$rc" + assert_eq "container: runner" "container" "$SQLITE_RUNNER" + assert_eq "container: count" "3" "$RETAIN_COUNT" + STUB_CONTAINER_SQLITE=0 + + # A sqlite3 that cannot bind (.parameter prints help to stdout, exit 0, + # parameter left NULL) must be rejected by the probe, not trusted. + FAKE_BIN="$FIXTURE_DIR/fakebin"; mkdir -p "$FAKE_BIN" + cat >"$FAKE_BIN/sqlite3" <<'FAKE' +#!/usr/bin/env bash +cat >/dev/null +echo ".parameter CMD ... Manage SQL parameter bindings" +echo "0" +FAKE + chmod +x "$FAKE_BIN/sqlite3" + reset_stub; STUB_PATH="$FAKE_BIN:$PATH" + read_retain_count >/dev/null 2>&1; rc=$? + assert_eq "unbindable sqlite3: rejected" "1" "$rc" + assert_eq "unbindable sqlite3: no runner" "" "$SQLITE_RUNNER" + assert_eq "unbindable sqlite3: no count" "" "$RETAIN_COUNT" + STUB_PATH="$PATH" + + # A query error on the target is a failure with no count, never "0". + reset_stub; TARGET_DB_PATH="$LEGACY_DB" + read_retain_count >/dev/null 2>&1; rc=$? + assert_eq "remote schema error: rc" "1" "$rc" + assert_eq "remote schema error: count" "" "$RETAIN_COUNT" + assert_true "remote schema error: stderr kept" contains "$(cat "$TMP/sqlite.err")" "no such column: from_pubkey" + TARGET_DB_PATH="$DB" # run_sqlite with no resolved runner must refuse rather than guess. SQLITE_RUNNER="" From 15f6c597b48b14778e6e8adc1a1e1edffb39ab1a Mon Sep 17 00:00:00 2001 From: Openclaw Date: Wed, 23 Sep 2026 14:28:07 +0200 Subject: [PATCH 3/5] fix(qa): exit non-zero when blacklist-test.sh is interrupted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit teardown was installed as `trap teardown EXIT INT TERM` and took its exit status from $?. On SIGINT/SIGTERM that is the status of whatever ran before the signal, usually 0, so an aborted run tore down correctly and then exited 0 — indistinguishable from a pass. Found by forcing a SIGTERM mid-run in the disposable SSH/Docker QA environment. Signal traps now pass 130 (INT) / 143 (TERM) to teardown, which still adds 1 if teardown itself fails. The traps live in install_teardown_traps so the unit test drives the real installation: clean exit, failure counts, teardown failure, and SIGTERM with and without a failing teardown. Co-Authored-By: Claude Opus 5.5 --- qa/scripts/blacklist-test.sh | 15 +++++++++++++-- qa/scripts/test-blacklist-sql.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/qa/scripts/blacklist-test.sh b/qa/scripts/blacklist-test.sh index 205ff8f7e..b94b5772c 100755 --- a/qa/scripts/blacklist-test.sh +++ b/qa/scripts/blacklist-test.sh @@ -32,7 +32,8 @@ # there is no fallback to interpolated SQL. # teardown-failed→ post-test removal did not restore listing # -# Exit code = number of failures (0 = pass). +# Exit code = number of failures (0 = pass). An interrupted run still tears +# down, then exits 130 (SIGINT) or 143 (SIGTERM), plus 1 if teardown failed. # PUBLIC repo: zero PII — no real pubkeys, IPs, or hostnames as defaults. # # Structure: helpers live at top level and the imperative body lives in main(), @@ -49,6 +50,10 @@ ssh_t() { ssh "${SSH_OPTS[@]}" "$TARGET_SSH_HOST" "$@"; } # ----------------------------------------------------------------------------- teardown() { local rc=$? + # Signal traps pass the conventional 128+signo. Without it $? is the status + # of whatever ran before the signal — usually 0 — and an aborted run would + # exit as a pass. + if [[ -n "${1:-}" ]]; then rc=$1; fi if [[ "$TEARDOWN_DONE" == "1" ]]; then rm -rf "$TMP"; exit "$rc"; fi TEARDOWN_DONE=1 echo "=== teardown: removing $TEST_PUBKEY from nodeBlacklist ===" @@ -67,6 +72,12 @@ teardown() { exit "$rc" } +install_teardown_traps() { + trap teardown EXIT + trap 'teardown 130' INT + trap 'teardown 143' TERM +} + # ----------------------------------------------------------------------------- # Helpers # ----------------------------------------------------------------------------- @@ -339,7 +350,7 @@ main() { TMP=$(mktemp -d) fails=0 TEARDOWN_DONE=0 - trap teardown EXIT INT TERM + install_teardown_traps # --------------------------------------------------------------------------- # §10.1 — hide diff --git a/qa/scripts/test-blacklist-sql.sh b/qa/scripts/test-blacklist-sql.sh index 4dfdc8b4b..8b888b8cc 100755 --- a/qa/scripts/test-blacklist-sql.sh +++ b/qa/scripts/test-blacklist-sql.sh @@ -283,5 +283,36 @@ FAKE fi fi +# ----- teardown exit status ---------------------------------------------------- +# Drives the script's own install_teardown_traps in a child bash with the side +# effects stubbed. The run must always tear down, and an interrupted run must +# never exit 0: before the fix, SIGTERM tore down and then exited as a pass. +TD_DIR=$(mktemp -d) +teardown_case() { # MODE(term|exit) FAILS NODE_VISIBLE_RC → prints exit status + : >"$TD_DIR/calls" + bash -c ' + calls="$2/calls"; visible_rc=$3; fails=$4; mode=$5 + . "$1" + remove_from_blacklist() { echo remove >>"$calls"; } + restart_target() { :; }; wait_for_stats() { :; } + node_visible() { return "$visible_rc"; } + TMP=$(mktemp -d); TEST_PUBKEY=synthetic; TEARDOWN_DONE=0 + install_teardown_traps + case "$mode" in + term) kill -TERM $$; sleep 5; exit 0 ;; + exit) exit "$fails" ;; + esac + ' _ "$SCRIPT_DIR/blacklist-test.sh" "$TD_DIR" "$3" "$2" "$1" >/dev/null 2>&1 + echo $? +} +assert_eq "clean run exits 0" "0" "$(teardown_case exit 0 0)" +assert_eq "clean run tore down once" "1" "$(grep -c remove "$TD_DIR/calls")" +assert_eq "two failures exit 2" "2" "$(teardown_case exit 2 0)" +assert_eq "teardown failure adds 1" "3" "$(teardown_case exit 2 1)" +assert_eq "SIGTERM mid-run exits 143" "143" "$(teardown_case term 0 0)" +assert_eq "SIGTERM still tore down (once)" "1" "$(grep -c remove "$TD_DIR/calls")" +assert_eq "SIGTERM + failed teardown" "144" "$(teardown_case term 0 1)" +rm -rf "$TD_DIR" + echo "test-blacklist-sql.sh: $PASS passed, $FAIL failed" [ "$FAIL" -eq 0 ] From fd479771e47628e5c7d4006a1a309a77c1af51d8 Mon Sep 17 00:00:00 2001 From: Openclaw Date: Wed, 23 Sep 2026 14:35:33 +0200 Subject: [PATCH 4/5] reviewfix(qa): finish teardown even if signalled again Independent review of the previous commit found that a second Ctrl-C or TERM arriving while teardown restores the target re-entered teardown through the TEARDOWN_DONE branch and exited at once: the node could stay blacklisted on the target, with no teardown-failed line and the earlier status lost. teardown now ignores INT/TERM once it starts; its steps are already bounded by CURL_TIMEOUT / RESTART_WAIT_S and SIGKILL still works. Tests: SIGINT maps to 130 and still tears down; a signal raised during teardown neither aborts it nor replaces the exit status. In CI a missing sqlite3 now fails instead of skipping the binding and error groups, and the fixture pubkey is validated before the reference count uses it. Co-Authored-By: Claude Opus 5.5 --- qa/scripts/blacklist-test.sh | 5 +++++ qa/scripts/test-blacklist-sql.sh | 28 ++++++++++++++++++++++------ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/qa/scripts/blacklist-test.sh b/qa/scripts/blacklist-test.sh index b94b5772c..d8d2f32be 100755 --- a/qa/scripts/blacklist-test.sh +++ b/qa/scripts/blacklist-test.sh @@ -34,6 +34,7 @@ # # Exit code = number of failures (0 = pass). An interrupted run still tears # down, then exits 130 (SIGINT) or 143 (SIGTERM), plus 1 if teardown failed. +# Further INT/TERM are ignored while teardown restores the target. # PUBLIC repo: zero PII — no real pubkeys, IPs, or hostnames as defaults. # # Structure: helpers live at top level and the imperative body lives in main(), @@ -56,6 +57,10 @@ teardown() { if [[ -n "${1:-}" ]]; then rc=$1; fi if [[ "$TEARDOWN_DONE" == "1" ]]; then rm -rf "$TMP"; exit "$rc"; fi TEARDOWN_DONE=1 + # Restoring the target must not be cut short by a second Ctrl-C/TERM, or the + # node would stay blacklisted with no warning. Each step below is bounded by + # CURL_TIMEOUT / RESTART_WAIT_S; SIGKILL still stops a hung run. + trap '' INT TERM echo "=== teardown: removing $TEST_PUBKEY from nodeBlacklist ===" if remove_from_blacklist && restart_target && wait_for_stats; then if node_visible; then diff --git a/qa/scripts/test-blacklist-sql.sh b/qa/scripts/test-blacklist-sql.sh index 8b888b8cc..ffdc49d0d 100755 --- a/qa/scripts/test-blacklist-sql.sh +++ b/qa/scripts/test-blacklist-sql.sh @@ -106,7 +106,11 @@ assert_match "ingestor DDL has from_pubkey" "from_pubkey[[:space:]]+TEXT" "$DDL" assert_true "ingestor DDL has no from_node" lacks "$DDL" "from_node" # ----- against a real sqlite3 ---------------------------------------------- -if ! command -v sqlite3 >/dev/null 2>&1; then +if ! command -v sqlite3 >/dev/null 2>&1 && [ -n "${CI:-}" ]; then + # In CI a missing sqlite3 must not turn the binding and error-surfacing + # groups into a silent pass. + FAIL=$((FAIL + 1)); echo "FAIL: sqlite3 not on PATH in CI — the query group cannot run" >&2 +elif ! command -v sqlite3 >/dev/null 2>&1; then echo "SKIP: sqlite3 not on PATH — skipping the ${#SQLITE_ARGS[@]}-flag query group" >&2 echo " (the alphabet, query-text and schema assertions above still ran)" >&2 else @@ -182,8 +186,10 @@ SQL if [ -f "$REAL_FIXTURE" ]; then cp "$REAL_FIXTURE" "$FIXTURE_DIR/real.db" real_pk=$(run_local "$FIXTURE_DIR/real.db" <<<"SELECT from_pubkey FROM transmissions WHERE from_pubkey IS NOT NULL GROUP BY from_pubkey ORDER BY COUNT(*) DESC, from_pubkey LIMIT 1;") - real_n=$(run_local "$FIXTURE_DIR/real.db" <<<"SELECT COUNT(*) FROM transmissions WHERE from_pubkey = '$real_pk';") assert_match "real fixture has an attributed pubkey" '^[0-9a-f]{64}$' "$real_pk" + # Only interpolated into the reference count once it is known to be hex. + [[ "$real_pk" =~ ^[0-9a-f]{64}$ ]] || real_pk="" + real_n=$(run_local "$FIXTURE_DIR/real.db" <<<"SELECT COUNT(*) FROM transmissions WHERE from_pubkey = '$real_pk';") assert_match "real fixture count > 0" '^[1-9][0-9]*$' "$real_n" assert_eq "script query on real fixture" "$real_n" "$(count "$real_pk" "$FIXTURE_DIR/real.db")" else @@ -288,19 +294,25 @@ fi # effects stubbed. The run must always tear down, and an interrupted run must # never exit 0: before the fix, SIGTERM tore down and then exited as a pass. TD_DIR=$(mktemp -d) -teardown_case() { # MODE(term|exit) FAILS NODE_VISIBLE_RC → prints exit status +# MODE: exit | term | int | exit-sig-in-teardown. The last one signals the run +# while teardown is restoring the target; teardown must still finish. +teardown_case() { # MODE FAILS NODE_VISIBLE_RC → prints exit status : >"$TD_DIR/calls" bash -c ' calls="$2/calls"; visible_rc=$3; fails=$4; mode=$5 . "$1" remove_from_blacklist() { echo remove >>"$calls"; } - restart_target() { :; }; wait_for_stats() { :; } - node_visible() { return "$visible_rc"; } + restart_target() { :; } + wait_for_stats() { + if [ "$mode" = exit-sig-in-teardown ]; then kill -TERM $$; kill -INT $$; fi + } + node_visible() { echo visible-checked >>"$calls"; return "$visible_rc"; } TMP=$(mktemp -d); TEST_PUBKEY=synthetic; TEARDOWN_DONE=0 install_teardown_traps case "$mode" in term) kill -TERM $$; sleep 5; exit 0 ;; - exit) exit "$fails" ;; + int) kill -INT $$; sleep 5; exit 0 ;; + exit|exit-sig-in-teardown) exit "$fails" ;; esac ' _ "$SCRIPT_DIR/blacklist-test.sh" "$TD_DIR" "$3" "$2" "$1" >/dev/null 2>&1 echo $? @@ -312,6 +324,10 @@ assert_eq "teardown failure adds 1" "3" "$(teardown_case exit 2 1)" assert_eq "SIGTERM mid-run exits 143" "143" "$(teardown_case term 0 0)" assert_eq "SIGTERM still tore down (once)" "1" "$(grep -c remove "$TD_DIR/calls")" assert_eq "SIGTERM + failed teardown" "144" "$(teardown_case term 0 1)" +assert_eq "SIGINT mid-run exits 130" "130" "$(teardown_case int 0 0)" +assert_eq "SIGINT still tore down (once)" "1" "$(grep -c remove "$TD_DIR/calls")" +assert_eq "signal during teardown: status kept" "2" "$(teardown_case exit-sig-in-teardown 2 0)" +assert_eq "signal during teardown: teardown finished" "1" "$(grep -c visible-checked "$TD_DIR/calls")" rm -rf "$TD_DIR" echo "test-blacklist-sql.sh: $PASS passed, $FAIL failed" From 8a0a420e28ec5e3125a40ffe3f49778ac7279053 Mon Sep 17 00:00:00 2001 From: Openclaw Date: Wed, 23 Sep 2026 14:38:47 +0200 Subject: [PATCH 5/5] reviewfix(qa): keep teardown steps interruptible Follow-up review of fd479771: `trap '' INT TERM` is inherited by every child, so once teardown started, the ssh steps ignored Ctrl-C too, and only curl is actually time-bounded (ConnectTimeout covers connection setup, not a hung session). A stuck `docker restart` or a dropped connection left the operator with SIGQUIT/SIGKILL, both of which skip the rest of teardown. teardown now installs a handler that only reports the signal. Children keep the default disposition, so a terminal Ctrl-C stops the step in progress; that step fails and teardown reports teardown-failed instead of exiting silently. SSH gains ServerAliveInterval=15/CountMax=4 so a dead session fails within about a minute. Tests: a child started during teardown must still die on SIGINT (it signals itself; trap -p cannot show an inherited ignore), and SIGINT with a failing teardown must exit 131, which distinguishes the INT trap from bash's own default 130. Co-Authored-By: Claude Opus 5.5 --- qa/scripts/blacklist-test.sh | 18 ++++++++++++------ qa/scripts/test-blacklist-sql.sh | 7 ++++++- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/qa/scripts/blacklist-test.sh b/qa/scripts/blacklist-test.sh index d8d2f32be..195cdd5b5 100755 --- a/qa/scripts/blacklist-test.sh +++ b/qa/scripts/blacklist-test.sh @@ -34,7 +34,8 @@ # # Exit code = number of failures (0 = pass). An interrupted run still tears # down, then exits 130 (SIGINT) or 143 (SIGTERM), plus 1 if teardown failed. -# Further INT/TERM are ignored while teardown restores the target. +# A further INT/TERM does not abort teardown: it stops only the step in +# progress (e.g. a hung ssh), which is then reported as teardown-failed. # PUBLIC repo: zero PII — no real pubkeys, IPs, or hostnames as defaults. # # Structure: helpers live at top level and the imperative body lives in main(), @@ -57,10 +58,12 @@ teardown() { if [[ -n "${1:-}" ]]; then rc=$1; fi if [[ "$TEARDOWN_DONE" == "1" ]]; then rm -rf "$TMP"; exit "$rc"; fi TEARDOWN_DONE=1 - # Restoring the target must not be cut short by a second Ctrl-C/TERM, or the - # node would stay blacklisted with no warning. Each step below is bounded by - # CURL_TIMEOUT / RESTART_WAIT_S; SIGKILL still stops a hung run. - trap '' INT TERM + # A second Ctrl-C/TERM must not abort the restore, or the node would stay + # blacklisted with no warning. Note the signal rather than ignoring it: an + # ignored disposition is inherited by ssh/curl, so a hung step could no longer + # be interrupted. With a handler, a terminal Ctrl-C still stops the running + # step; that step fails and teardown reports teardown-failed. + trap 'echo " (signal received — teardown continues restoring the target)" >&2' INT TERM echo "=== teardown: removing $TEST_PUBKEY from nodeBlacklist ===" if remove_from_blacklist && restart_target && wait_for_stats; then if node_visible; then @@ -350,7 +353,10 @@ main() { CURL_TIMEOUT="${CURL_TIMEOUT:-60}" RESTART_WAIT_S="${RESTART_WAIT_S:-120}" - SSH_OPTS=(-i "$TARGET_SSH_KEY" -o StrictHostKeyChecking=accept-new -o ConnectTimeout=15 -o BatchMode=yes) + # ServerAlive*: a dead connection mid-command fails after ~60s instead of + # hanging; ConnectTimeout only bounds connection setup. + SSH_OPTS=(-i "$TARGET_SSH_KEY" -o StrictHostKeyChecking=accept-new -o ConnectTimeout=15 -o BatchMode=yes + -o ServerAliveInterval=15 -o ServerAliveCountMax=4) TMP=$(mktemp -d) fails=0 diff --git a/qa/scripts/test-blacklist-sql.sh b/qa/scripts/test-blacklist-sql.sh index ffdc49d0d..de24fe1f0 100755 --- a/qa/scripts/test-blacklist-sql.sh +++ b/qa/scripts/test-blacklist-sql.sh @@ -302,7 +302,10 @@ teardown_case() { # MODE FAILS NODE_VISIBLE_RC → prints exit status calls="$2/calls"; visible_rc=$3; fails=$4; mode=$5 . "$1" remove_from_blacklist() { echo remove >>"$calls"; } - restart_target() { :; } + # A child started during teardown must still die on SIGINT, or a hung ssh + # could not be interrupted. It signals itself; "survived" means the + # disposition was inherited as ignored (trap -p cannot show that). + restart_target() { echo "child-int:$(sh -c "kill -INT \$\$; echo survived" 2>/dev/null)" >>"$calls"; } wait_for_stats() { if [ "$mode" = exit-sig-in-teardown ]; then kill -TERM $$; kill -INT $$; fi } @@ -326,8 +329,10 @@ assert_eq "SIGTERM still tore down (once)" "1" "$(grep -c remove "$TD_DIR/call assert_eq "SIGTERM + failed teardown" "144" "$(teardown_case term 0 1)" assert_eq "SIGINT mid-run exits 130" "130" "$(teardown_case int 0 0)" assert_eq "SIGINT still tore down (once)" "1" "$(grep -c remove "$TD_DIR/calls")" +assert_eq "SIGINT + failed teardown" "131" "$(teardown_case int 0 1)" assert_eq "signal during teardown: status kept" "2" "$(teardown_case exit-sig-in-teardown 2 0)" assert_eq "signal during teardown: teardown finished" "1" "$(grep -c visible-checked "$TD_DIR/calls")" +assert_eq "teardown children still die on SIGINT" "child-int:" "$(grep child-int "$TD_DIR/calls")" rm -rf "$TD_DIR" echo "test-blacklist-sql.sh: $PASS passed, $FAIL failed"