Skip to content

fix(qa): bind blacklist pubkey in SQLite query (port of upstream #1982) - #82

Merged
dborup merged 6 commits into
masterfrom
codex/port-upstream-1982-bind-blacklist-sql
Sep 23, 2026
Merged

dborup merged 6 commits into
masterfrom
codex/port-upstream-1982-bind-blacklist-sql

Conversation

@adminopenclaw8-sketch

@adminopenclaw8-sketch adminopenclaw8-sketch commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • stop interpolating TEST_PUBKEY into the blacklist retention query; bind it as an SQLite parameter via a byte-safe hex literal
  • probe the target's sqlite3 for working parameter binding instead of silently falling back; no interpolating fallback exists
  • surface query errors separately from a legitimate zero-row result
  • count against the real column, transmissions.from_pubkey — the query previously named from_node, which no CoreScope database has
  • exit non-zero when the script is interrupted — SIGINT/SIGTERM used to tear down and then exit 0, i.e. report a pass

The from_node blocker

The §10.2 probe counted SELECT COUNT(*) FROM transmissions WHERE from_node = :pubkey. The column does not exist, so against any real CoreScope database the probe could only fail with no such column: from_node. The unit test hid this because it created its own table with an invented from_node column. (The column name predates this PR; the old interpolated query used it too.)

Schema evidence (master f52bf7d5)

Fact Where
Base schema: transmissions(… from_pubkey TEXT …); no from_node anywhere in the schema cmd/ingestor/db.go:348-360
Migration from_pubkey_v1 adds the column + idx_transmissions_from_pubkey on older DBs cmd/ingestor/db.go:803-818, internal/dbschema/dbschema.go:419-432
Server refuses to start unless the column exists internal/dbschema/dbschema.go:158 (mustCol("transmissions", "from_pubkey"))
Written only for ADVERTs, from the decoded pubkey cmd/ingestor/db.go:2576-2580; backfill cmd/ingestor/maintenance.go:188-310
Value is hex.EncodeToString of the 32-byte key → 64 lowercase hex chars cmd/ingestor/decoder.go:329
Server readers use exact match on the same column cmd/server/db.go:866, 915, 1197, 4355, cmd/server/advert_stats.go:66
nodeBlacklist matching is case-insensitive (ToLower + TrimSpace) cmd/server/config.go:927-941, 977-996
Only view (packets_v) does not alter transmissions cmd/ingestor/db.go:456

Since stored values are lowercase but the script's hex gate and the blacklist accept any case, the query is now WHERE from_pubkey = lower(:pubkey) — the value is still a bound parameter; only its normalisation happens in SQL. The count is "ADVERT transmissions attributed to this node", which is what the ingestor can attribute (encrypted payloads carry no sender).

Changes in this update

Binding, stdin transport, the capability probe, the container→host fallback, visible SQL errors and token-via-stdin handling are unchanged. No production code or schema is touched.

Verification

Unit tests — bash qa/scripts/test-blacklist-sql.sh

Environment Result
macOS, bash 3.2.57, sqlite3 3.51.0 85 passed, 0 failed
Alpine, bash 5.3.9, sqlite3 3.53.4, CI=true 85 passed, 0 failed
Ubuntu 24.04, bash 5.2.21, /bin/sh = dash, no sqlite3 29 passed, 0 failed (sqlite group skipped locally; signal/teardown group ran under dash)
CI=true with no sqlite3 on PATH fails (27 passed, 1 failed) instead of silently skipping

Coverage added: query text names from_pubkey and not from_node; DDL comes from the ingestor; legitimate pubkey → its rows (also upper-case input); absent pubkey and pubkey prefix → 0; seven injection payloads bind literally (the interpolated form leaks all 6 rows); empty, whitespace, multibyte and 10,000-byte values; real captured fixture count equals a direct count; missing column / missing table → non-zero exit + named on stderr + no count; a stubbed ssh_t that runs the remote command through a real bash -c proves container→host fallback, container runner, rejection of an sqlite3 that cannot bind, a remote schema error kept as a failure, and that SQL and the (hex-encoded) pubkey travel on stdin and never in the remote argv; teardown exit codes for clean, failing, failed-teardown and SIGTERM runs.

Mutation checks (each must make the suite fail)

Mutation Suite result
query reverted to from_node = :pubkey 46 failed
binding replaced by lower('%s') interpolation 17 failed (') OR 1=1 -- leaks all 6 rows)
sqlite3 error swallowed into a count of 0 10 failed
ingestor DDL without from_pubkey 39 failed
SQL passed as a remote argv argument instead of stdin 16 failed
trap teardown EXIT INT TERM (old trap) 6 failed
trap teardown INT (no 130) 3 failed
INT trap removed entirely 1 failed (131 expected, 130 got)
trap '' INT TERM in teardown (inherited ignore) 1 failed (child survived SIGINT)
teardown signal handler removed 2 failed

All ten were run against the final test file; the unmutated suite passes.

Full SSH/Docker run in an isolated, disposable environment

Run from a disposable environment on the shared demo host with its own resource prefix; nothing belonging to other work was touched, and no staging or production system was contacted.

  • Candidate: image built from this branch with the repository Dockerfile (no sqlite3, as in production), plus a test-only variant with the sqlite package added for the container-runner case
  • Isolation: app container on an --internal Docker network (outbound blocked, verified), DISABLE_MOSQUITTO / DISABLE_CADDY, no MQTT sources, its own data directory and config.json; reachable only through a proxy published on the host's loopback
  • Data: a synthetic 32-byte pubkey with three ADVERT transmissions, plus one unrelated synthetic node; no real identities or tokens (the token used was synthetic)
  • SSH/Docker path: the unmodified blacklist-test.sh ran on the demo host and reached a disposable sshd "target host" container over real OpenSSH (ephemeral key, isolated known_hosts); that container drove the real app container through the Docker socket (docker restart, docker exec -i … sqlite3) and edited the real bind-mounted config.json

Final pass on 8a0a420e:

Run Scenario Exit Outcome
A no sqlite3 in the app image or on the target 1 hide ok, topology clean, classified retain-failed: no sqlite3 able to bind, teardown ok
B app image without sqlite3, target with it 0 hide ok, runner host, 3 rows retained, teardown ok
C app image with sqlite3 0 hide ok, runner container, 3 rows retained, teardown ok
D SIGTERM mid-run (during the post-restart wait) 143 teardown ok
E SIGTERM mid-run, second SIGTERM once teardown had started 143 teardown ok

For every run: before and after, the node was visible (detail 200, in list), nodeBlacklist was [] with an identical config hash, the app container was running, and the transmission counts were unchanged (4 total, 3 for the target). The unrelated node's row was never counted.

Process arguments were captured completely with strace -f -e trace=execve over the script's process tree (plus a host-wide ps sampler): the SQL text, the column name and the token appeared in 0 argv entries in all runs. Logs were sanitised; the synthetic pubkey does not occur in them. The environment (stopped containers, networks, images, data and logs) is kept for inspection.

Other checks

  • bash -n on both scripts, git diff --check
  • .github/workflows/deploy.yml parsed with Ruby Psych; against master it differs only by this PR's unit-test step
  • cd cmd/server && go test -run 'ForkGuard|Workflow' . — pass
  • independent review by a separate reviewer (three passes): no blockers; the should-fix findings are addressed in fd479771 and 8a0a420e

Known limitations

  1. The pubkey still appears in process arguments through paths this PR does not touch: the config-edit ssh (PK=… bash -s), the curl URLs (/api/nodes/<pk>, and the admin probe below) and two grep patterns — 9 execve entries per full run. It is the node's public identifier, served by the API itself; the SQL path, which is what this PR changes, does not expose it. Moving these to stdin would be a separate change.
  2. The ADMIN_API_TOKEN path queries /api/admin/transmissions?from_node=…. No such endpoint exists in cmd/server, so it always falls through to the SQLite probe. Pre-existing; the token itself stays out of argv.
  3. TARGET_DB_PATH is used for both runners. In the container runner it is a container path; for the host fallback it must be a host path. In the demo both views mounted the data at the same path. When they differ, the host fallback fails as retain-failed (never a false pass), and sqlite3 may create an empty file at that path if its directory exists on the host (-readonly was considered but not adopted here).
  4. The count is ADVERT transmissions attributed via from_pubkey; a node that has never sent an ADVERT reports 0 and fails §10.2, as intended.
  5. Review nits left open: if stderr alone is a broken pipe, the teardown signal message can raise SIGPIPE and end teardown early; and Ctrl-C during the post-restart wait stops only the current poll, which still ends at RESTART_WAIT_S.
  6. The demo host refused SSH port forwarding for the available key, so the runner executed on that host against a disposable sshd container rather than from a workstation against a separate machine. The production image has no sqlite3, so the operator's target host needs the sqlite3 CLI for §10.2.
  7. shellcheck was not available locally.

Upstream

Fork port of upstream CoreScope PR 1982 (issue 1977), applied to fork master and extended here with the from_pubkey and exit-status fixes, which are fork-local.

Workflow safety

The workflow diff against master is only the local SQL unit-test step. Fork guards for GHCR publishing, releases, staging deployment and badge writes are unchanged. No staging, production or upstream system was contacted.

🤖 Generated with Claude Code

Openclaw and others added 6 commits September 23, 2026 07:04
Brings in #78, #80 and #81 so the SQL-binding fix is validated against
current master.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Follow-up review of fd47977: `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 <noreply@anthropic.com>
@dborup
dborup merged commit 6334c42 into master Sep 23, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants