From 289de239912b660142aa9f9c90caf830a9286f35 Mon Sep 17 00:00:00 2001 From: Anupam Mediratta Date: Mon, 7 Sep 2026 07:35:33 +0530 Subject: [PATCH 1/2] refactor(qa): make blacklist-test.sh sourceable for unit tests Move the imperative body of the script into main() and only run it when the file is executed directly, so a test can source the file and exercise individual helpers without running the QA suite against a live target. This is the idiom already used by scripts/staging/disk-monitor.sh:99, which scripts/staging/test-disk-monitor.sh relies on. Behaviour is unchanged. The moved assignments are deliberately not made `local`: bash variables are global unless declared otherwise, so the top-level helpers keep seeing TARGET_CONTAINER, TMP, CURL_TIMEOUT etc. exactly as before, and a trap installed inside main() is still process-wide. Verified movement-only by comparing the sorted, whitespace- stripped line multiset before and after: no original line changed. Groundwork for #1977. Co-Authored-By: Claude Opus 5 --- qa/scripts/blacklist-test.sh | 238 +++++++++++++++++++---------------- 1 file changed, 127 insertions(+), 111 deletions(-) diff --git a/qa/scripts/blacklist-test.sh b/qa/scripts/blacklist-test.sh index 94b5b4127..ff6d0705f 100755 --- a/qa/scripts/blacklist-test.sh +++ b/qa/scripts/blacklist-test.sh @@ -30,60 +30,16 @@ # # 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 +62,6 @@ teardown() { rm -rf "$TMP" exit "$rc" } -trap teardown EXIT INT TERM # ----------------------------------------------------------------------------- # Helpers @@ -197,75 +152,136 @@ node_visible() { } # ----------------------------------------------------------------------------- -# §10.1 — hide +# main # ----------------------------------------------------------------------------- -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 +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 -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 + 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:-}" -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 + 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 -# ----------------------------------------------------------------------------- -# §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 "") + # 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 -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" + # 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 "") + echo " ✅ topology clean" + fi + + # --------------------------------------------------------------------------- + # §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 "") + 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" + 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 fi 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 + 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 -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 From 71fa4449e3794feaa66bbe3b325f29a8e7640040 Mon Sep 17 00:00:00 2001 From: Anupam Mediratta Date: Mon, 7 Sep 2026 07:59:29 +0530 Subject: [PATCH 2/2] fix(qa): bind TEST_PUBKEY as a SQLite parameter instead of interpolating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §10.2 built its query as SELECT COUNT(*) FROM transmissions WHERE from_node = '$TEST_PUBKEY'; so the SQL layer's safety rested entirely on the outer hex gate rather than on the SQL layer itself. Bind the value instead. The value is hex-encoded and bound as `cast(x'..' as text)` rather than passed to `.parameter set` as a quoted string. Dot-command arguments are split on whitespace, so a payload containing a space makes sqlite3 print the .parameter help to *stdout*, exit 0, and leave the parameter unbound — COUNT(*) then returns 0, which reads exactly like a passing security fix. -bail does not catch it. Hex encoding removes the quoting layer entirely: the value's contribution to the SQL text is drawn from [0-9a-f] only, for arbitrary input rather than only for hex-gated input. Capability is probed, not versioned: bind a known token and read it back, on the operator's binary rather than one we pin. If neither the container nor the host qualifies, fail loudly naming what is needed. There is no interpolating fallback — that would leave the vulnerable path in place under a nicer name. The hex gate is kept as defence in depth, and the exit status and stderr are no longer discarded, so a broken query is distinguishable from a legitimately empty result. Also fixes a double-count in §10.2: the "TARGET_DB_PATH unset" branch incremented $fails and then left count="", so the generic branch incremented it a second time for the same failure. Tests assert both directions — a legitimate pubkey still returns its row (a zero from a command that failed proves nothing), the payload returns 0 while the table holds 2 rows, the old interpolated form leaked all 2, and a missing table exits non-zero with a message on stderr. Refs #1977 Co-Authored-By: Claude Opus 5 --- .github/workflows/deploy.yml | 3 + qa/scripts/blacklist-test.sh | 168 +++++++++++++++++++++++++------ qa/scripts/test-blacklist-sql.sh | 135 +++++++++++++++++++++++++ 3 files changed, 274 insertions(+), 32 deletions(-) create mode 100755 qa/scripts/test-blacklist-sql.sh diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index feeb48b40..6bd08b500 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -166,6 +166,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 ff6d0705f..535f477f0 100755 --- a/qa/scripts/blacklist-test.sh +++ b/qa/scripts/blacklist-test.sh @@ -25,7 +25,10 @@ # 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). @@ -151,6 +154,127 @@ node_visible() { return 1 } +# ----------------------------------------------------------------------------- +# §10.2 DB probe — bind the pubkey, do not interpolate it (issue #1977) +# ----------------------------------------------------------------------------- +# 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 + +# 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')" +} + +# 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' +} + +# 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 + + 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 # ----------------------------------------------------------------------------- @@ -175,7 +299,10 @@ main() { exit 2 fi - # Hard input validation — these strings are interpolated into remote shell/SQL. + # 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 @@ -241,38 +368,15 @@ main() { # §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 "") - 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" - 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 - fi - fi - - if [[ -z "$count" ]]; then - echo " ❌ retain-failed: could not read transmissions count" + 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 [[ "$count" =~ ^[0-9]+$ ]] && (( count > 0 )); then - echo " ✅ DB retains $count packets from $TEST_PUBKEY" + elif [[ "$RETAIN_COUNT" =~ ^[0-9]+$ ]] && (( RETAIN_COUNT > 0 )); then + echo " ✅ DB retains $RETAIN_COUNT packets from $TEST_PUBKEY" else - echo " ❌ retain-failed: count=$count (expected > 0)" + echo " ❌ retain-failed: count=$RETAIN_COUNT (expected > 0)" fails=$((fails+1)) 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 ]