fix(server): filter blacklisted nodes from /api/analytics/topology

[dborup]

Tracked in issue #91 (separate from the QA hardening in #87, which detects this bug).

Privacy bug

GET /api/analytics/topology returned nodes that are listed in nodeBlacklist. It exposed their pubkey, name, repeater rank, pairings and per-observer reachability, even though the node endpoints hide them correctly.

Root cause

filterBlacklistedFromTopology type-asserted []TopRepeater, []TopPair, []BestPathEntry, []MultiObsNode and map[string]*ObserverReach. computeAnalyticsTopology builds those parts as []map[string]interface{} / map[string]interface{}. Every assertion missed, each branch was skipped, and the data went out unfiltered. No server test covered the blacklist on this route.

A second hazard sat under it. The handler receives the store's shared cached object (the steady-state recomputer snapshot, or a topoCache entry), and the filter wrote its result back into that map. A filter that matched would have modified the cache in place and raced with concurrent readers.

Fix (cmd/server/topology_privacy.go, one central filter)

• Works on the store's real shape. It also reads JSON-decoded ([]interface{}) and typed-struct shapes, so a change in how the store builds the result cannot quietly turn the filter back into a no-op.
• Filters the five pubkey-bearing parts:
  • topRepeaters[].pubkey
  • topPairs[].pubkeyA / pubkeyB (the pair goes if either side is hidden)
  • bestPathList[].pubkey
  • multiObsNodes[].pubkey
  • perObserverReach{}.rings[].nodes[].pubkey
• Same matching as every other privacy check: Config.IsBlacklisted (trimmed, case-insensitive), plus HiddenNamePrefixes (Feature request: Hide and remove nodes from database with the emoji 🚫 in the beginning Kpa-clawbot/CoreScope#1181) on the entry's name.
• Fails closed: a part, ring, entry, pubkey or name the filter cannot read is dropped, never passed on. Missing keys are left missing.
• Never writes to its input: copy-on-write per part, observer, ring and entry list. With nothing to hide it returns the input unchanged, so the response is byte-identical and the filter allocates nothing.
• Race-free handler gate: it uses the new Config.HasNodeBlacklist(), which reads the same atomic set as IsBlacklisted, instead of len(cfg.NodeBlacklist). The gate also runs when only HiddenNamePrefixes is configured, as the other privacy-filtered handlers do (routes.go subpath/detail).
• No per-entry database lookup: the entry already carries the node's resolved name, from the same nodes.name column.

The topology computation, and responses for nodes that are not hidden, are unchanged.

Red → green (test-first)

cmd/server/topology_blacklist_filter_test.go drives the real handler over data built by the real computeAnalyticsTopology. A seed places a target repeater in all five parts: topPairs as both pubkeyA and pubkeyB, multiObsNodes via two observers, and perObserverReach for both. Every test first proves the target is present before blacklisting, so an "absent afterwards" result can never pass vacuously.

• f116d414 (tests only) against master's code: every test fails with the intended assertions (target still in all six positions, unreadable shapes leak it, input mutated). No precondition fails.
• ba7922ce / 342864b7 / 90cff688: green.

Coverage:

• lower-, upper- and mixed-case and padded entries; empty, nil and blank blacklists (response unchanged); multiple nodes; each pair side;
• hidden-name prefixes with and without a blacklist; unresolved hops (nil pubkey) kept; non-string names dropped;
• a missing single part; JSON-decoded and typed shapes; 14 unknown/malformed shapes (fail closed);
• the input, the topoCache entry and the recomputer snapshot are never modified; cold (startup config) and warm requests; window queries;
• concurrent requests, blacklist changes and cache invalidation; no database lookups.

Mutations

18 targeted mutants, all killed on the intended assertion (checked on the final head):

• no filtering;
• pairs checked on the A side only / B side only;
• each of topRepeaters, bestPathList, multiObsNodes, perObserverReach skipped;
• case normalisation removed;
• whitespace trim removed;
• filter expects the wrong type;
• a stale filtered result served after a blacklist change;
• filtering writes into the shared cache;
• unknown shape passed through;
• the seed changed so the target is absent before blacklisting (caught by the precondition);
• the old len(cfg.NodeBlacklist) gate (caught as a DATA RACE);
• a name of an unexpected type kept;
• nil pubkey treated as hidden;
• a per-entry isPubkeyHidden lookup added back.

An independent reviewer ran 35 more mutants. Every non-equivalent survivor was closed in 2adf1ae8/90cff688.

Race / cache

• Full cmd/server suite under -race: green on 342864b7 (328 s) and on the final 90cff688 (266 s).
• TestTopologyBlacklist_Concurrent: 12 readers × 15 requests racing cache eviction, in two phases (blacklisted / not). No response ever disagrees with the blacklist in force. A third phase toggles SetNodeBlacklist against live requests under -race.
• Filtering happens per response on the unfiltered cached data, so a blacklist change needs no cache invalidation and cannot serve stale privacy state.

Performance

Same seeded store (200 nodes, 2000 paths), medians of interleaved runs. The filter runs only when a blacklist or hidden-name prefixes are configured.

Case	Result
No blacklist, warm or cold	Same code path as master: 6921 allocs both ways; timing within run-to-run noise
Blacklist set, nothing matches	Filter alone ≈ 31 µs per call for ~535 entries (≈58 ns/entry), 0 allocs; warm endpoint allocs equal to master (6921)
Target hidden	≈ 34 µs, 30–32 allocs, and a smaller response
Only hidden-name prefixes	≈ 11 µs, 0 allocs, no database queries (an intermediate version made one SQLite query per bestPathList entry; caught in review)

The cost is dominated by the shared Config.IsBlacklisted normalisation.

End-to-end with the merged QA script (#87)

Run in an isolated Docker environment on the demo host, not staging. It used an --internal network with no published ports (outbound verified blocked), synthetic data, and apps built from master 1ee44d72 and from this branch. The script ran through a runner container and a disposable sshd target, both under strace.

Topology probe (target in topRepeaters / pairsA / pairsB / bestPath / multiObs / reach):

• Before blacklisting, both builds: true ×6, 24 pubkey fields.
• Blacklisted with an upper-case entry, master: true ×6 (the leak).
• Blacklisted with an upper-case entry, this branch: false ×6, 13 other fields intact.
• Restored: true ×6 again.

qa/scripts/blacklist-test.sh, using an upper-case TEST_NODE_PUBKEY:

Build	Result	Exit
master	❌ hide-failed: /api/analytics/topology lists the blacklisted pubkey	1 (red)
this branch	✅ topology clean (13 pubkey fields checked)	0 (green)
this branch, SIGTERM mid-run	torn down and restored	143

After every run:

• the config was byte-identical to the original (unsorted list with a duplicate and an upper-case entry, mode 640 root:root);
• transmissions and files were unchanged;
• the target was back in all six positions.

Across the final runs: 452 execve (runner and target sshd), with 0 hits for the pubkey (either case), SELECT, from_pubkey, /api/nodes or /api/analytics.

Verification

• go build, go vet ./..., and gofmt on the changed files are clean.
• git diff --check is clean.
• The QA suite (qa/scripts/test-blacklist-sql.sh) passes 849/0.
• The full cmd/server suite passes under -race.
• No workflow or QA-script changes.
• Master moved to 08716f43 (test(reach-rank): wait for the remounted board before typing; prove Back restores the search #90, a single e2e test file) during the work. There is no overlap, the merge is clean, and build, vet, the relevant tests under -race and the QA suite pass on the merge result.

Independent review

A fresh reviewer who did not write the fix found no blockers. Its should-fix items are fixed and re-confirmed (no blockers):

• a per-entry DB lookup in bestPathList;
• two missing fail-closed/kept tests.

It confirmed that dropping isPubkeyHidden is safe, because the entry name and the stored name come from the same nodes.name column.

Limitations / follow-ups (not in this PR)

• Observer IDs. observers[].id, the perObserverReach keys and the observer_id fields hold observer IDs, which can be node pubkeys. A node-blacklisted node that is also an observer still appears there. This matches /api/observers, which only checks the observer blacklist. Whether node blacklisting should cover observers is a product decision.
• Residual traces. Unresolved hops keep their 1–2-byte prefixes, and a hidden node's traffic still counts in aggregate stats. Top-N lists shrink rather than refill.
• Rename-to-hide lag. A node that renames itself to a hidden prefix is hidden once the topology is recomputed (≤ 5 min for the snapshot). A pubkey blacklist applies on every request.
• QA script precondition. The QA script reports "topology clean" without checking that the test node was in the topology before blacklisting. That is a follow-up for qa/scripts/blacklist-test.sh.
• Test-only race. isPubkeyHidden elsewhere reads HiddenNamePrefixes without the atomic. That only matters for tests that call SetHiddenNamePrefixes concurrently.

🤖 Generated with Claude Code