Skip to content

fix(qa): bind TEST_PUBKEY as a SQLite parameter instead of interpolating it #1982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anupamme wants to merge 2 commits into
base: master
Choose a base branch
from
+369 −111
Open

fix(qa): bind TEST_PUBKEY as a SQLite parameter instead of interpolating it #1982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
289de23
refactor(qa): make blacklist-test.sh sourceable for unit tests
anupamme Sep 7, 2026
71fa444
fix(qa): bind TEST_PUBKEY as a SQLite parameter instead of interpolat…
anupamme Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/deploy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
342 changes: 231 additions & 111 deletions qa/scripts/blacklist-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
# -----------------------------------------------------------------------------
Expand All @@ -106,7 +65,6 @@ teardown() {
rm -rf "$TMP"
exit "$rc"
}
trap teardown EXIT INT TERM

# -----------------------------------------------------------------------------
# Helpers
Expand Down Expand Up @@ -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 '<value>'`:
# 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
Loading