mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-10-23 21:12:40 +00:00
fix(qa): bind TEST_PUBKEY as a SQLite parameter instead of interpolating it (#1982)
Closes #1977. Supersedes #1952. Follow-up filed as #1983. ## What §10.2 did ```bash q="SELECT COUNT(*) FROM transmissions WHERE from_node = '$TEST_PUBKEY';" qq=$(printf %q "$q") if ! count=$(ssh_t "docker exec … sqlite3 … $qq" 2>/dev/null); then count=$(ssh_t "sqlite3 … $qq" 2>/dev/null || echo "") fi ``` The injection is not reachable today — `TEST_PUBKEY` is hex-gated and the script `exit 2`s before the SQL is built. The problem is that the SQL layer's safety rests entirely on that outer gate rather than on the SQL layer itself. #1952 proposed doubling embedded quotes; that is string escaping, not parameterisation, which is why it was withdrawn in favour of this. ## What this does Per the four points in the sign-off on #1977: **1. Bind the value.** A constant `SELECT` and a bound `:pubkey`, fed to sqlite3 on stdin. The SQL no longer crosses the remote shell as a command word, so there is no `printf %q` on the query at all any more. **Why hex rather than `.parameter set :pk '<value>'`.** Dot-command arguments are split on whitespace, so a payload containing a space produces too many arguments — and sqlite3 responds by printing the `.parameter` help to **stdout**, exiting **0**, and leaving `:pk` **unbound**. `COUNT(*)` then returns 0, which reads exactly like a passing security fix. `-bail` does not catch it. Verified on 3.51.0: ``` $ printf ".parameter set :pk '' OR 1=1 --'\nSELECT COUNT(*) FROM transmissions WHERE from_node = :pk;\n" \ | sqlite3 -bail ptest.db .parameter CMD ... Manage SQL parameter bindings # <- help, on stdout … 0 # <- :pk never bound $ echo $? 0 ``` `.parameter set :pk 1+1` also binds the integer `2` — the value is evaluated as an SQL expression and only falls back to a text literal when evaluation fails. So interpolating into the `.parameter set` line trades one hazard for another. Hex-encoding removes the quoting layer instead of adding one: the value is bound as `cast(x'<hex>' as text)`, so its contribution to the SQL text is drawn from the alphabet `[0-9a-f]` only. Nothing to quote, no tokenizer arity hazard, and it holds for **arbitrary** input rather than only for hex-gated input — which is the point. Verified against a fixture table holding two rows, one of them `deadbeef`: | value | result | exit | |---|---|---| | `deadbeef`, bound as `cast(x'6465616462656566' as text)` | `1` | 0 | | `' OR 1=1 --`, bound the same way | `0` | 0 | | `' OR 1=1 --`, interpolated the current way | `2` (whole table) | 0 | | query against a DB with no `transmissions` table, `-bail` | `Parse error … no such table` on **stderr** | **1** | **2. Probe the capability, not a version.** `resolve_sqlite_runner` binds `corescope-probe-ok` and asserts it comes back — a round trip, not a bare `.parameter init`, so the positive control runs against the operator's actual binary rather than one we pin. If neither the container nor the host qualifies, it fails loudly and names what is needed: ``` ❌ retain-failed: no sqlite3 able to bind a parameter on the target tried: docker exec -i corescope-prod sqlite3, then sqlite3 on runner@example need: the sqlite3 CLI reachable over ssh, supporting '.parameter set' OCI runtime exec failed: exec: "sqlite3": executable file not found in $PATH bash: line 1: sqlite3: command not found ``` There is deliberately **no** interpolating fallback. That would leave the vulnerable path in place under a nicer name. **3. The hex gate is kept**, with its comment updated to say why: for the SQL layer it is now defence in depth rather than the only guard. Redundant is not the same as wrong. **4. The exit status and stderr survive.** `-batch -bail -init /dev/null -noheader -list` (stop at the first SQL error; ignore the operator's `~/.sqliterc`, where a stray `.mode` would make the count unparseable; stdout is exactly the number). Query stderr is captured and printed on failure rather than sent to `/dev/null`, so a broken query is distinguishable from a legitimately empty result. Probe stderr is collected too, and printed only if *both* probes fail — the container miss is the known-normal case, so surfacing it on every run would be noise. ## Also fixed An existing 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. `read_retain_count` now gives §10.2 exactly one increment point. Opportunistic cleanup in a file already being touched (AGENTS.md line 318). ## Tests New `qa/scripts/test-blacklist-sql.sh`, wired into the `go-test` job. 24 assertions, modelled on `scripts/staging/test-disk-monitor.sh`. Both directions are asserted, because a zero from a command that failed proves nothing: - **Positive control** — `deadbeef` still returns its row (`1`, exit 0), and so does `cafebabe`; an absent pubkey returns `0`. - **Negative** — `' OR 1=1 --` returns `0` while the table demonstrably holds 2 rows, and the old interpolated form is asserted to leak all `2`. That last assertion is what makes the `0` above worth something. - **Error surfacing** — the same SQL against a DB with no `transmissions` table exits non-zero with a message on stderr and nothing on stdout. - **Alphabet** — `sql_hex_literal` output matches `^x'[0-9a-f]*'$` for the SQL payloads, a backslash, `$(id)` / backticks, an embedded newline, `héllo`, and a 4096-byte repetitive string. That last one is a regression guard for `od -v`: without the flag `od` collapses repeated identical lines to `*`. - `run_sqlite` with no resolved runner refuses rather than guessing. Group 2 skips loudly (rather than silently) if `sqlite3` is not on PATH; group 1 needs no sqlite3 and always runs. **Mutation-tested** — each of these breaks the suite, so the assertions have teeth: | mutation | caught by | |---|---| | restore full interpolation | `injection payload → 0 rows — expected '0' got '2'` | | naive `.parameter set '%s'` | `expected '0' got '.parameter CMD ...'` | | drop `od -v` | alphabet failure on `*`, plus `expected '8192' got '33'` | Commit 1 is a behaviour-neutral refactor that moves the imperative body into `main()` behind a `BASH_SOURCE` guard, so the test can source the script and exercise individual helpers. Same idiom as `scripts/staging/disk-monitor.sh:99`. ## Verification - `bash qa/scripts/test-blacklist-sql.sh` → 24 passed, 0 failed - `bash -n` on both scripts - All three runtime paths exercised end to end with PATH shims for `ssh`/`docker`/`sqlite3` against a real fixture DB: success (`sqlite3 runner: host`, count 2), query failure (classified message + `Parse error … no such table`, `fails=1`), and no-capability (the loud block above, both probe stderrs, `fails=1` — not 2) - The new step lands inside `go-test`, which runs when `changes.outputs.code == 'true'`; `qa/scripts/*.sh` does not match that job's `^docs/|[.]md$|^LICENSE$` documentation filter, so it is not skipped ## Deliberately out of scope - **The `docker exec` branch is dead on current images** → filed as #1983. The app container has no `sqlite3` at all: `Dockerfile:15` is pure-Go SQLite with no CGO, and the `apk add` installs only `mosquitto mosquitto-clients supervisor caddy wget`. So the host "fallback" is the only path that has ever executed, silently, because both branches discarded stderr. This change keeps both branches and merely makes the outcome visible (`sqlite3 runner: …` on every run). - **`-readonly` on the target DB.** Tempting, and verified compatible with `.parameter` (the binding table lives in the TEMP database), but a WAL database needing journal recovery can refuse a read-only open. Adding it here risks exactly the "trades an unreachable injection for a script that does not run" outcome flagged in the #1952 thread. Worth its own issue. - **The other `2>/dev/null` sites** in this file, which also sit awkwardly with `qa/README.md`'s "Don't silence stderr". Only the §10.2 lines named in the sign-off are touched. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Anupam MedirattaandClaude Opus 5 authored and GitHub committed
1 parent
3fbff01f64
commit
de237fc29c
3 files changed
+369
-111
No files matched your search
@@ -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
|
||||
|
||||
+231
-111
@@ -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 '<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
|
||||
Executable
+135
@@ -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 >/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 ]
|
||||
Reference in new issue
Block a user