From f116d41462860cf1cee1603430af750c84e75dbf Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Thu, 24 Sep 2026 17:09:24 +0200 Subject: [PATCH 1/5] test(server): reproduce blacklisted nodes in /api/analytics/topology (#91) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handler tests over data built by the real computeAnalyticsTopology (maps, not the Topology* structs). A seed places a target repeater in all five pubkey-bearing parts — topRepeaters, topPairs as pubkeyA and pubkeyB, bestPathList, multiObsNodes and perObserverReach for two observers — and every test first proves the target is present before blacklisting it. Covers case/whitespace variants, empty blacklists, multiple nodes, both pair sides, hidden-name prefixes, the topoCache and recomputer-snapshot paths (never modified by filtering), window queries, concurrent requests with blacklist changes and cache invalidation, a missing part, other known shapes, and unknown shapes that must fail closed. All fail on master 1ee44d72: the target stays in every part. Co-Authored-By: Claude Opus 5.5 --- cmd/server/topology_blacklist_filter_test.go | 658 +++++++++++++++++++ 1 file changed, 658 insertions(+) create mode 100644 cmd/server/topology_blacklist_filter_test.go diff --git a/cmd/server/topology_blacklist_filter_test.go b/cmd/server/topology_blacklist_filter_test.go new file mode 100644 index 000000000..3ce43b079 --- /dev/null +++ b/cmd/server/topology_blacklist_filter_test.go @@ -0,0 +1,658 @@ +package main + +// /api/analytics/topology must not surface blacklisted nodes. +// +// These tests drive the real handler over data built by the real +// computeAnalyticsTopology (maps, not the Topology* structs), because that is +// where the filter used to fail: it type-asserted []TopRepeater and friends, +// every assertion missed, and the blacklisted node came back in all five +// pubkey-bearing parts of the response. +// +// Every test first proves the target IS in the topology without a blacklist. +// A target that was never there would make "absent after blacklisting" pass +// vacuously. + +import ( + "encoding/json" + "fmt" + "net/http/httptest" + "reflect" + "sort" + "strings" + "sync" + "testing" + "time" + + "github.com/gorilla/mux" +) + +const ( + topoTarget = "7a7a000000000001" // in every part, as pubkeyA and pubkeyB + topoLower = "5b5b000000000002" // pairs with the target as pubkeyA + topoUpper = "8c8c000000000003" // pairs with the target as pubkeyB + topoOther = "9d9d000000000004" // never blacklisted +) + +// topoPositions are the places a node's pubkey can appear in the response. +var topoPositions = []string{ + "topRepeaters", "topPairs.pubkeyA", "topPairs.pubkeyB", + "bestPathList", "multiObsNodes", "perObserverReach", +} + +// seedTopologyPrivacyDB builds a store whose computed topology holds the +// target in every position: four repeaters with distinct 1-byte hop prefixes, +// paths seen by two different observers (multiObsNodes needs >1 observer per +// hop), and pairs where the target sorts first and where it sorts second. +func seedTopologyPrivacyDB(t *testing.T) *DB { + t.Helper() + db := setupTestDB(t) + now := time.Now().UTC() + recent := now.Add(-1 * time.Hour).Format(time.RFC3339) + epoch := now.Add(-1 * time.Hour).Unix() + mustExec := func(q string, args ...interface{}) { + t.Helper() + if _, err := db.conn.Exec(q, args...); err != nil { + t.Fatalf("seed: %v", err) + } + } + mustExec(`INSERT INTO observers (id, name, iata, last_seen, first_seen, packet_count) VALUES ('obs1','Observer One','SJC',?,'2026-01-01T00:00:00Z',10)`, recent) + mustExec(`INSERT INTO observers (id, name, iata, last_seen, first_seen, packet_count) VALUES ('obs2','Observer Two','SFO',?,'2026-01-01T00:00:00Z',10)`, recent) + for _, n := range [][2]string{{topoTarget, "Target"}, {topoLower, "Lower"}, {topoUpper, "Upper"}, {topoOther, "Other"}} { + mustExec(`INSERT INTO nodes (public_key, name, role, lat, lon, last_seen, first_seen, advert_count) VALUES (?, ?, 'repeater', 37.5, -122.0, ?, '2026-01-01T00:00:00Z', 5)`, n[0], n[1], recent) + } + for i, p := range []struct { + path string + obs int + }{ + {`["5b","7a","8c"]`, 1}, // pairs 5b|7a (target = B) and 7a|8c (target = A) + {`["7a","9d"]`, 2}, + {`["9d","5b"]`, 1}, + {`["9d","7a"]`, 2}, + } { + mustExec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) VALUES ('AABB', ?, ?, 1, 5, '{"type":"CHAN"}')`, fmt.Sprintf("topo-priv-%d", i), recent) + mustExec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) VALUES (?, ?, 10, -90, ?, ?)`, i+1, p.obs, p.path, epoch) + } + return db +} + +func setupTopologyPrivacyServer(t *testing.T, blacklist []string) (*Server, *mux.Router) { + t.Helper() + db := seedTopologyPrivacyDB(t) + cfg := &Config{Port: 3000, NodeBlacklist: blacklist} + srv := NewServer(db, cfg, NewHub()) + store := NewPacketStore(db, nil) + if err := store.Load(); err != nil { + t.Fatalf("store.Load: %v", err) + } + if !store.WaitIndexesReady(5 * time.Second) { + t.Fatalf("indexes never became ready") + } + store.config = cfg + srv.store = store + router := mux.NewRouter() + srv.RegisterRoutes(router) + return srv, router +} + +func getTopology(t *testing.T, router *mux.Router, query string) map[string]interface{} { + t.Helper() + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("GET", "/api/analytics/topology"+query, nil)) + if w.Code != 200 { + t.Fatalf("topology: HTTP %d: %s", w.Code, w.Body.String()) + } + var body map[string]interface{} + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("topology: not JSON: %v", err) + } + return body +} + +// topoPubkeys returns the lower-cased pubkeys at each position of a decoded +// response. A part that is missing or has the wrong shape is a test failure: +// the response contract is what clients (and the QA script) rely on. +func topoPubkeys(t *testing.T, body map[string]interface{}) map[string][]string { + t.Helper() + out := map[string][]string{} + add := func(pos string, v interface{}) { + if s, ok := v.(string); ok { + out[pos] = append(out[pos], strings.ToLower(s)) + } + } + list := func(k string) []interface{} { + v, ok := body[k] + if !ok { + t.Fatalf("response has no %q", k) + } + if v == nil { + return nil + } + l, ok := v.([]interface{}) + if !ok { + t.Fatalf("%q is %T, want a list", k, v) + } + return l + } + for _, e := range list("topRepeaters") { + add("topRepeaters", e.(map[string]interface{})["pubkey"]) + } + for _, e := range list("topPairs") { + m := e.(map[string]interface{}) + add("topPairs.pubkeyA", m["pubkeyA"]) + add("topPairs.pubkeyB", m["pubkeyB"]) + } + for _, e := range list("bestPathList") { + add("bestPathList", e.(map[string]interface{})["pubkey"]) + } + for _, e := range list("multiObsNodes") { + add("multiObsNodes", e.(map[string]interface{})["pubkey"]) + } + reach, ok := body["perObserverReach"].(map[string]interface{}) + if !ok { + t.Fatalf("perObserverReach is %T, want an object", body["perObserverReach"]) + } + for _, obs := range reach { + rings, _ := obs.(map[string]interface{})["rings"].([]interface{}) + for _, r := range rings { + nodes, _ := r.(map[string]interface{})["nodes"].([]interface{}) + for _, n := range nodes { + add("perObserverReach", n.(map[string]interface{})["pubkey"]) + } + } + } + return out +} + +func topoHas(list []string, s string) bool { + for _, v := range list { + if v == s { + return true + } + } + return false +} + +// requireEverywhere is the precondition: without it, "absent" proves nothing. +func requireEverywhere(t *testing.T, body map[string]interface{}, pk string) { + t.Helper() + got := topoPubkeys(t, body) + for _, pos := range topoPositions { + if !topoHas(got[pos], pk) { + t.Fatalf("precondition: %s not in %s before blacklisting (%v) — the test would prove nothing", pk, pos, got[pos]) + } + } +} + +func requireNowhere(t *testing.T, body map[string]interface{}, pk string) { + t.Helper() + got := topoPubkeys(t, body) + for _, pos := range topoPositions { + if topoHas(got[pos], pk) { + t.Errorf("blacklisted %s still in %s", pk, pos) + } + } +} + +// visibleEntries is every entry of the five parts that references none of the +// hidden pubkeys, as canonical JSON — what filtering must leave untouched. +func visibleEntries(t *testing.T, body map[string]interface{}, hidden ...string) []string { + t.Helper() + refs := func(m map[string]interface{}, keys ...string) bool { + for _, k := range keys { + if s, ok := m[k].(string); ok && topoHas(hidden, strings.ToLower(s)) { + return true + } + } + return false + } + var out []string + keep := func(part string, m map[string]interface{}) { + b, _ := json.Marshal(m) + out = append(out, part+":"+string(b)) + } + for _, part := range []string{"topRepeaters", "bestPathList", "multiObsNodes"} { + l, _ := body[part].([]interface{}) + for _, e := range l { + if m := e.(map[string]interface{}); !refs(m, "pubkey") { + keep(part, m) + } + } + } + pairs, _ := body["topPairs"].([]interface{}) + for _, e := range pairs { + if m := e.(map[string]interface{}); !refs(m, "pubkeyA", "pubkeyB") { + keep("topPairs", m) + } + } + reach, _ := body["perObserverReach"].(map[string]interface{}) + for obsID, obs := range reach { + rings, _ := obs.(map[string]interface{})["rings"].([]interface{}) + for _, r := range rings { + ring := r.(map[string]interface{}) + nodes, _ := ring["nodes"].([]interface{}) + for _, n := range nodes { + if m := n.(map[string]interface{}); !refs(m, "pubkey") { + keep(fmt.Sprintf("perObserverReach[%s][%v]", obsID, ring["hops"]), m) + } + } + } + } + sort.Strings(out) + return out +} + +// ─── Reproduction ──────────────────────────────────────────────────────────── + +func TestTopologyBlacklist_RemovesTargetFromEveryPart(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + before := getTopology(t, router, "") + requireEverywhere(t, before, topoTarget) + + srv.cfg.SetNodeBlacklist([]string{topoTarget}) + after := getTopology(t, router, "") + requireNowhere(t, after, topoTarget) + if b, a := visibleEntries(t, before, topoTarget), visibleEntries(t, after, topoTarget); !reflect.DeepEqual(b, a) { + t.Errorf("entries not involving the blacklisted node changed:\nbefore %v\nafter %v", b, a) + } + // Other top-level fields are not privacy-filtered and must not change. + for _, k := range []string{"uniqueNodes", "avgHops", "medianHops", "maxHops", "hopDistribution", "hopsVsSnr", "observers"} { + if !reflect.DeepEqual(before[k], after[k]) { + t.Errorf("%s changed: %v → %v", k, before[k], after[k]) + } + } +} + +// Configured at startup (the production path: a config change needs a +// restart), so the very first, cold computation is filtered too. +func TestTopologyBlacklist_ColdStartConfig(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, []string{topoTarget}) + requireNowhere(t, getTopology(t, router, ""), topoTarget) // cold + requireNowhere(t, getTopology(t, router, ""), topoTarget) // warm (cached) + // The target is really in the computed data: clearing the blacklist shows it. + srv.cfg.SetNodeBlacklist(nil) + requireEverywhere(t, getTopology(t, router, ""), topoTarget) +} + +// Same normalisation as every other privacy check (Config.IsBlacklisted): +// trimmed, case-insensitive. +func TestTopologyBlacklist_CaseAndWhitespace(t *testing.T) { + for _, entry := range []string{ + topoTarget, + strings.ToUpper(topoTarget), + "7A7a000000000001", + " " + strings.ToUpper(topoTarget) + "\t", + } { + t.Run(fmt.Sprintf("%q", entry), func(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + requireEverywhere(t, getTopology(t, router, ""), topoTarget) + srv.cfg.SetNodeBlacklist([]string{entry}) + requireNowhere(t, getTopology(t, router, ""), topoTarget) + }) + } +} + +func TestTopologyBlacklist_EmptyBlacklistChangesNothing(t *testing.T) { + for name, bl := range map[string][]string{"nil": nil, "empty": {}, "blank entry": {" "}} { + t.Run(name, func(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + before := getTopology(t, router, "") + requireEverywhere(t, before, topoTarget) + srv.cfg.SetNodeBlacklist(bl) + after := getTopology(t, router, "") + if !reflect.DeepEqual(before, after) { + t.Errorf("response changed with blacklist %q", bl) + } + }) + } +} + +func TestTopologyBlacklist_MultipleNodes(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + before := getTopology(t, router, "") + requireEverywhere(t, before, topoTarget) + got := topoPubkeys(t, before) + if !topoHas(got["topPairs.pubkeyA"], topoLower) || !topoHas(got["topPairs.pubkeyB"], topoUpper) { + t.Fatalf("precondition: %s as pubkeyA and %s as pubkeyB expected, got %v", topoLower, topoUpper, got) + } + srv.cfg.SetNodeBlacklist([]string{topoTarget, strings.ToUpper(topoLower), topoUpper}) + after := getTopology(t, router, "") + for _, pk := range []string{topoTarget, topoLower, topoUpper} { + requireNowhere(t, after, pk) + } + if !topoHas(topoPubkeys(t, after)["topRepeaters"], topoOther) { + t.Errorf("non-blacklisted %s disappeared", topoOther) + } + if b, a := visibleEntries(t, before, topoTarget, topoLower, topoUpper), visibleEntries(t, after, topoTarget, topoLower, topoUpper); !reflect.DeepEqual(b, a) { + t.Errorf("entries not involving blacklisted nodes changed:\nbefore %v\nafter %v", b, a) + } +} + +// A blacklisted node on only one side of a pair removes the pair — whichever +// side it is on. +func TestTopologyBlacklist_PairSides(t *testing.T) { + for _, tc := range []struct{ pk, side string }{{topoLower, "pubkeyA"}, {topoUpper, "pubkeyB"}} { + t.Run(tc.side, func(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + before := getTopology(t, router, "") + if !topoHas(topoPubkeys(t, before)["topPairs."+tc.side], tc.pk) { + t.Fatalf("precondition: %s not in topPairs.%s", tc.pk, tc.side) + } + srv.cfg.SetNodeBlacklist([]string{tc.pk}) + after := topoPubkeys(t, getTopology(t, router, "")) + if topoHas(after["topPairs.pubkeyA"], tc.pk) || topoHas(after["topPairs.pubkeyB"], tc.pk) { + t.Errorf("pair with blacklisted %s on side %s kept", tc.pk, tc.side) + } + }) + } +} + +// Name-based hiding (HiddenNamePrefixes, #1181) applies to the same entries, +// with or without a node blacklist configured. +func TestTopologyBlacklist_HiddenNamePrefix(t *testing.T) { + for name, bl := range map[string][]string{"prefixes only": nil, "with a blacklist": {"ffff000000000000"}} { + t.Run(name, func(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + requireEverywhere(t, getTopology(t, router, ""), topoTarget) + srv.cfg.SetNodeBlacklist(bl) + srv.cfg.SetHiddenNamePrefixes([]string{"Targ"}) + requireNowhere(t, getTopology(t, router, ""), topoTarget) + if !topoHas(topoPubkeys(t, getTopology(t, router, ""))["topRepeaters"], topoOther) { + t.Errorf("a node with a visible name was hidden") + } + }) + } +} + +// ─── Caches: filtered per response, never in place ─────────────────────────── + +func TestTopologyBlacklist_CacheIsNotMutated(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + requireEverywhere(t, getTopology(t, router, ""), topoTarget) // warms topoCache + cachedBefore, _ := json.Marshal(srv.store.GetAnalyticsTopology("", "")) + + srv.cfg.SetNodeBlacklist([]string{topoTarget}) + requireNowhere(t, getTopology(t, router, ""), topoTarget) + cachedAfter, _ := json.Marshal(srv.store.GetAnalyticsTopology("", "")) + if string(cachedBefore) != string(cachedAfter) { + t.Fatalf("the shared cached topology was modified by filtering") + } + // No stale privacy state in either direction. + srv.cfg.SetNodeBlacklist(nil) + requireEverywhere(t, getTopology(t, router, ""), topoTarget) + srv.cfg.SetNodeBlacklist([]string{topoTarget}) + requireNowhere(t, getTopology(t, router, ""), topoTarget) +} + +// The steady-state path serves the recomputer's snapshot (issue #1240) — the +// same shared-object rules apply to it. +func TestTopologyBlacklist_RecomputerSnapshot(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + store := srv.store + rc := newAnalyticsRecomputer("topo-privacy-test", time.Hour, func() interface{} { + return store.computeAnalyticsTopology("", "", TimeWindow{}) + }) + rc.runOnce() + store.analyticsRecomputerMu.Lock() + store.recompTopology = rc + store.analyticsRecomputerMu.Unlock() + snapBefore, _ := json.Marshal(rc.Load()) + + requireEverywhere(t, getTopology(t, router, ""), topoTarget) + srv.cfg.SetNodeBlacklist([]string{topoTarget}) + requireNowhere(t, getTopology(t, router, ""), topoTarget) + if snapAfter, _ := json.Marshal(rc.Load()); string(snapBefore) != string(snapAfter) { + t.Fatalf("the recomputer snapshot was modified by filtering") + } + if runs := rc.ComputeRuns(); runs != 1 { + t.Errorf("filtering recomputed the snapshot (%d runs)", runs) + } +} + +// Region/window queries take the non-snapshot cache path. +func TestTopologyBlacklist_WindowQuery(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + requireEverywhere(t, getTopology(t, router, "?days=7"), topoTarget) + srv.cfg.SetNodeBlacklist([]string{topoTarget}) + requireNowhere(t, getTopology(t, router, "?days=7"), topoTarget) +} + +// Concurrent requests, blacklist changes and cache invalidation: no data race +// (run with -race), and within each phase no response disagrees with the +// blacklist in force. +func TestTopologyBlacklist_Concurrent(t *testing.T) { + srv, router := setupTopologyPrivacyServer(t, nil) + requireEverywhere(t, getTopology(t, router, ""), topoTarget) + + fetch := func() (map[string]interface{}, error) { + w := httptest.NewRecorder() + router.ServeHTTP(w, httptest.NewRequest("GET", "/api/analytics/topology", nil)) + if w.Code != 200 { + return nil, fmt.Errorf("HTTP %d", w.Code) + } + var body map[string]interface{} + return body, json.Unmarshal(w.Body.Bytes(), &body) + } + phase := func(blacklisted bool) { + t.Helper() + var wg sync.WaitGroup + errs := make(chan error, 256) + for g := 0; g < 12; g++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 15; i++ { + body, err := fetch() + if err != nil { + errs <- err + return + } + b, _ := json.Marshal(body) + if has := strings.Contains(string(b), topoTarget); has == blacklisted { + errs <- fmt.Errorf("blacklisted=%v but target present=%v", blacklisted, has) + return + } + } + }() + } + wg.Add(1) + go func() { // cache invalidation racing the readers + defer wg.Done() + for i := 0; i < 15; i++ { + srv.store.invalidateCachesFor(cacheInvalidation{eviction: true}) + } + }() + wg.Wait() + close(errs) + for err := range errs { + t.Error(err) + } + } + srv.cfg.SetNodeBlacklist([]string{topoTarget}) + phase(true) + srv.cfg.SetNodeBlacklist(nil) + phase(false) + + // Blacklist changes racing requests: only the race detector judges this. + var wg sync.WaitGroup + stop := make(chan struct{}) + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; ; i++ { + select { + case <-stop: + return + default: + } + if i%2 == 0 { + srv.cfg.SetNodeBlacklist([]string{topoTarget}) + } else { + srv.cfg.SetNodeBlacklist(nil) + } + } + }() + for g := 0; g < 8; g++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 10; i++ { + if _, err := fetch(); err != nil { + t.Error(err) + return + } + } + }() + } + time.Sleep(50 * time.Millisecond) + close(stop) + wg.Wait() +} + +// ─── The filter itself: shapes, fail-closed, no mutation ───────────────────── + +// computedTopology returns the real computeAnalyticsTopology output for the +// seed, plus a server whose blacklist holds the target. +func computedTopology(t *testing.T) (*Server, map[string]interface{}) { + t.Helper() + srv, _ := setupTopologyPrivacyServer(t, []string{topoTarget}) + return srv, srv.store.computeAnalyticsTopology("", "", TimeWindow{}) +} + +func jsonOf(t *testing.T, v interface{}) string { + t.Helper() + b, err := json.Marshal(v) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return string(b) +} + +func TestFilterBlacklistedFromTopology_DoesNotMutateInput(t *testing.T) { + srv, data := computedTopology(t) + before := jsonOf(t, data) + if !strings.Contains(before, topoTarget) { + t.Fatalf("precondition: computed topology lacks the target") + } + out := srv.filterBlacklistedFromTopology(data) + if after := jsonOf(t, data); after != before { + t.Fatalf("input topology was modified in place") + } + if strings.Contains(jsonOf(t, out), topoTarget) { + t.Errorf("filtered output still contains the target") + } +} + +func TestFilterBlacklistedFromTopology_NilAndNoMatch(t *testing.T) { + srv, data := computedTopology(t) + if got := srv.filterBlacklistedFromTopology(nil); got != nil { + t.Errorf("nil input → %v", got) + } + srv.cfg.SetNodeBlacklist([]string{"ffff000000000000"}) + out := srv.filterBlacklistedFromTopology(data) + if jsonOf(t, out) != jsonOf(t, data) { + t.Errorf("nothing blacklisted in the data, but the output differs") + } +} + +// One required key missing: the rest is still filtered and nothing panics. +func TestFilterBlacklistedFromTopology_MissingKey(t *testing.T) { + for _, key := range []string{"topRepeaters", "topPairs", "bestPathList", "multiObsNodes", "perObserverReach"} { + t.Run(key, func(t *testing.T) { + srv, data := computedTopology(t) + cp := map[string]interface{}{} + for k, v := range data { + if k != key { + cp[k] = v + } + } + out := srv.filterBlacklistedFromTopology(cp) + if _, ok := out[key]; ok { + t.Errorf("filter invented %q", key) + } + if strings.Contains(jsonOf(t, out), topoTarget) { + t.Errorf("target leaked with %q missing", key) + } + }) + } +} + +// JSON-decoded ([]interface{}) and typed (Topology* structs) shapes are +// filtered too, so a change in how the store builds the result cannot turn +// the filter into a no-op again. +func TestFilterBlacklistedFromTopology_OtherKnownShapes(t *testing.T) { + srv, data := computedTopology(t) + var decoded map[string]interface{} + if err := json.Unmarshal([]byte(jsonOf(t, data)), &decoded); err != nil { + t.Fatal(err) + } + if out := jsonOf(t, srv.filterBlacklistedFromTopology(decoded)); strings.Contains(out, topoTarget) || !strings.Contains(out, topoOther) { + t.Errorf("JSON-decoded shape: target present=%v other present=%v", strings.Contains(out, topoTarget), strings.Contains(out, topoOther)) + } + + typed := map[string]interface{}{ + "topRepeaters": []TopRepeater{{Hop: "7a", Pubkey: topoTarget, Name: "Target"}, {Hop: "9d", Pubkey: topoOther, Name: "Other"}}, + "topPairs": []TopPair{{HopA: "7a", HopB: "9d", PubkeyA: topoTarget, PubkeyB: topoOther}, {HopA: "5b", HopB: "9d", PubkeyA: topoLower, PubkeyB: topoOther}}, + "bestPathList": []BestPathEntry{{Hop: "7a", Pubkey: topoTarget}, {Hop: "9d", Pubkey: topoOther}}, + "multiObsNodes": []MultiObsNode{{Hop: "7a", Pubkey: topoTarget}, {Hop: "9d", Pubkey: topoOther}}, + "perObserverReach": map[string]*ObserverReach{"obs1": {ObserverName: "o", Rings: []ReachRing{{Hops: 1, Nodes: []ReachNode{ + {Hop: "7a", Pubkey: strings.ToUpper(topoTarget)}, {Hop: "9d", Pubkey: topoOther}}}}}}, + } + out := jsonOf(t, srv.filterBlacklistedFromTopology(typed)) + if strings.Contains(strings.ToLower(out), topoTarget) { + t.Errorf("typed shape: target leaked: %s", out) + } + if strings.Count(out, topoOther) != 5 { + t.Errorf("typed shape: non-blacklisted entries lost: %s", out) + } +} + +// Anything the filter cannot read is dropped, never passed through: an +// unknown shape must not become a privacy leak. Every case carries the +// target's pubkey somewhere the filter cannot interpret it, so a pass-through +// shows up as the pubkey in the output. +func TestFilterBlacklistedFromTopology_UnknownShapeFailsClosed(t *testing.T) { + wrapped := map[string]interface{}{"hex": topoTarget} // a pubkey field that is not a string + node := func(pk interface{}) map[string]interface{} { return map[string]interface{}{"hop": "7a", "pubkey": pk} } + reach := func(rings interface{}) map[string]interface{} { + return map[string]interface{}{"obs1": map[string]interface{}{"observer_name": "o", "rings": rings}} + } + cases := []struct { + name, part string + value interface{} + }{ + {"string instead of list", "topRepeaters", topoTarget}, + {"object instead of list", "bestPathList", map[string]interface{}{"pubkey": topoTarget}}, + {"list of strings", "multiObsNodes", []string{topoTarget}}, + {"list of non-objects", "topRepeaters", []interface{}{topoTarget}}, + {"pubkey not a string", "topRepeaters", []interface{}{node(wrapped)}}, + {"pubkey as list", "bestPathList", []map[string]interface{}{node([]interface{}{topoTarget})}}, + {"pubkeyA not a string", "topPairs", []interface{}{map[string]interface{}{"pubkeyA": wrapped, "pubkeyB": topoOther}}}, + {"pubkeyB not a string", "topPairs", []interface{}{map[string]interface{}{"pubkeyA": topoOther, "pubkeyB": wrapped}}}, + {"reach not an object", "perObserverReach", []interface{}{node(topoTarget)}}, + {"reach entry not an object", "perObserverReach", map[string]interface{}{"obs1": topoTarget}}, + {"rings not a list", "perObserverReach", reach(map[string]interface{}{"x": topoTarget})}, + {"ring not an object", "perObserverReach", reach([]interface{}{topoTarget})}, + {"nodes not a list", "perObserverReach", reach([]interface{}{map[string]interface{}{"hops": 1, "nodes": node(topoTarget)}})}, + {"reach node pubkey not a string", "perObserverReach", reach([]interface{}{map[string]interface{}{"hops": 1, "nodes": []interface{}{node(wrapped)}}})}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv, data := computedTopology(t) + cp := map[string]interface{}{} + for k, v := range data { + cp[k] = v + } + cp[tc.part] = tc.value + if !strings.Contains(jsonOf(t, cp), topoTarget) { + t.Fatalf("precondition: case does not carry the target") + } + out := jsonOf(t, srv.filterBlacklistedFromTopology(cp)) + if strings.Contains(strings.ToLower(out), topoTarget) { + t.Errorf("unreadable %s leaked the target: %s", tc.part, out) + } + if !strings.Contains(out, topoOther) { + t.Errorf("readable parts were dropped along with the unreadable one") + } + }) + } +} From ba7922cebe2d578601b2c0bf6f41390bf4ff88ab Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Thu, 24 Sep 2026 17:09:24 +0200 Subject: [PATCH 2/5] fix(server): filter blacklisted nodes from /api/analytics/topology (#91) filterBlacklistedFromTopology type-asserted []TopRepeater, []TopPair, []BestPathEntry, []MultiObsNode and map[string]*ObserverReach, but computeAnalyticsTopology builds maps and slices of maps, so every branch was skipped and blacklisted nodes were returned unfiltered. - One central filter (topology_privacy.go) that works on the shape the store produces, also reads JSON-decoded and typed shapes, and fails closed: a part or entry it cannot read is dropped, never passed on. - Matching uses Config.IsBlacklisted (trimmed, case-insensitive) and HiddenNamePrefixes; a pair goes if either side is hidden. - The handler gets the shared cached object (recomputer snapshot or topoCache); the filter never writes to it and copies only the parts that change. With nothing hidden the input is returned as is. - The handler gate uses the new Config.HasNodeBlacklist, which reads the atomic set (race-free against SetNodeBlacklist), and also runs the filter when only HiddenNamePrefixes are configured, as the other privacy-filtered handlers do. Co-Authored-By: Claude Opus 5.5 --- cmd/server/config.go | 28 +++- cmd/server/routes.go | 107 +------------- cmd/server/topology_privacy.go | 259 +++++++++++++++++++++++++++++++++ 3 files changed, 284 insertions(+), 110 deletions(-) create mode 100644 cmd/server/topology_privacy.go diff --git a/cmd/server/config.go b/cmd/server/config.go index 5388ca27d..f1660cbf0 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -975,13 +975,29 @@ func (c *Config) BlacklistGeneration() uint64 { // lazily on first read from c.NodeBlacklist (covering the JSON-load path // where the setter was never called). func (c *Config) IsBlacklisted(pubkey string) bool { - if c == nil { + set := c.blacklistSet() + if len(set) == 0 { return false } + return set[strings.ToLower(strings.TrimSpace(pubkey))] +} + +// HasNodeBlacklist reports whether at least one (non-blank) pubkey is +// blacklisted. It reads the same atomic set as IsBlacklisted, so unlike +// len(c.NodeBlacklist) it is safe against a concurrent SetNodeBlacklist. +func (c *Config) HasNodeBlacklist() bool { + return len(c.blacklistSet()) > 0 +} + +// blacklistSet returns the active normalised blacklist set (shared, +// read-only), materialising it lazily from the JSON-loaded slice on first +// read. CAS-style: if another goroutine wins the race, ours is dropped. +func (c *Config) blacklistSet() map[string]bool { + if c == nil { + return nil + } mp := c.blacklistSetPtr.Load() if mp == nil { - // Lazy first-read materialisation from the JSON-loaded slice. - // CAS-style: if another goroutine wins the race, drop ours. built := buildBlacklistSet(c.NodeBlacklist) if c.blacklistSetPtr.CompareAndSwap(nil, &built) { mp = &built @@ -989,10 +1005,10 @@ func (c *Config) IsBlacklisted(pubkey string) bool { mp = c.blacklistSetPtr.Load() } } - if mp == nil || len(*mp) == 0 { - return false + if mp == nil { + return nil } - return (*mp)[strings.ToLower(strings.TrimSpace(pubkey))] + return *mp } // IsNameHidden returns true if the given node name starts with any of the diff --git a/cmd/server/routes.go b/cmd/server/routes.go index e1312b91e..7133d510c 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -2773,8 +2773,10 @@ func (s *Server) handleAnalyticsTopology(w http.ResponseWriter, r *http.Request) return } } + // The store hands out its shared cached result; the filter never + // writes to it and returns a filtered copy when anything is hidden. data := s.store.GetAnalyticsTopologyWithWindow(region, area, window) - if s.cfg != nil && len(s.cfg.NodeBlacklist) > 0 { + if s.cfg != nil && (s.cfg.HasNodeBlacklist() || len(s.cfg.hiddenPrefixes()) > 0) { data = s.filterBlacklistedFromTopology(data) } writeJSON(w, data) @@ -4204,109 +4206,6 @@ func constantTimeEqual(a, b string) bool { return subtle.ConstantTimeCompare([]byte(a), []byte(b)) == 1 } -// filterBlacklistedFromTopology removes blacklisted + hidden-prefix node -// references (#1181) from the topology analytics response (TopRepeaters, -// TopPairs, BestPathList, MultiObsNodes, PerObserverReach). -func (s *Server) filterBlacklistedFromTopology(data map[string]interface{}) map[string]interface{} { - // Filter TopRepeaters - if repeaters, ok := data["topRepeaters"]; ok { - if arr, ok := repeaters.([]TopRepeater); ok { - var filtered []TopRepeater - for _, r := range arr { - if pk, ok := r.Pubkey.(string); ok && s.cfg.IsBlacklisted(pk) { - continue - } - if name, ok := r.Name.(string); ok && s.cfg.IsNameHidden(name) { - continue - } - filtered = append(filtered, r) - } - data["topRepeaters"] = filtered - } - } - - // Filter TopPairs - if pairs, ok := data["topPairs"]; ok { - if arr, ok := pairs.([]TopPair); ok { - var filtered []TopPair - for _, p := range arr { - if pkA, ok := p.PubkeyA.(string); ok && s.cfg.IsBlacklisted(pkA) { - continue - } - if pkB, ok := p.PubkeyB.(string); ok && s.cfg.IsBlacklisted(pkB) { - continue - } - if nameA, ok := p.NameA.(string); ok && s.cfg.IsNameHidden(nameA) { - continue - } - if nameB, ok := p.NameB.(string); ok && s.cfg.IsNameHidden(nameB) { - continue - } - filtered = append(filtered, p) - } - data["topPairs"] = filtered - } - } - - // Filter BestPathList - if paths, ok := data["bestPathList"]; ok { - if arr, ok := paths.([]BestPathEntry); ok { - var filtered []BestPathEntry - for _, p := range arr { - if pk, ok := p.Pubkey.(string); ok && s.cfg.IsBlacklisted(pk) { - continue - } - if pk, ok := p.Pubkey.(string); ok && s.isPubkeyHidden(pk) { - continue - } - filtered = append(filtered, p) - } - data["bestPathList"] = filtered - } - } - - // Filter MultiObsNodes - if nodes, ok := data["multiObsNodes"]; ok { - if arr, ok := nodes.([]MultiObsNode); ok { - var filtered []MultiObsNode - for _, n := range arr { - if pk, ok := n.Pubkey.(string); ok && s.cfg.IsBlacklisted(pk) { - continue - } - if name, ok := n.Name.(string); ok && s.cfg.IsNameHidden(name) { - continue - } - filtered = append(filtered, n) - } - data["multiObsNodes"] = filtered - } - } - - // Filter PerObserverReach - if reach, ok := data["perObserverReach"]; ok { - if m, ok := reach.(map[string]*ObserverReach); ok { - for k, v := range m { - for ri := range v.Rings { - var filteredNodes []ReachNode - for _, rn := range v.Rings[ri].Nodes { - if pk, ok := rn.Pubkey.(string); ok && s.cfg.IsBlacklisted(pk) { - continue - } - if name, ok := rn.Name.(string); ok && s.cfg.IsNameHidden(name) { - continue - } - filteredNodes = append(filteredNodes, rn) - } - v.Rings[ri].Nodes = filteredNodes - } - m[k] = v - } - } - } - - return data -} - // filterBlacklistedFromSubpaths removes blacklisted node references from // the subpaths analytics response. func (s *Server) filterBlacklistedFromSubpaths(data map[string]interface{}) map[string]interface{} { diff --git a/cmd/server/topology_privacy.go b/cmd/server/topology_privacy.go new file mode 100644 index 000000000..7329ae0e5 --- /dev/null +++ b/cmd/server/topology_privacy.go @@ -0,0 +1,259 @@ +package main + +import ( + "encoding/json" + "log" +) + +// Privacy filtering for /api/analytics/topology (issue #91). +// +// computeAnalyticsTopology builds its result from maps and slices of maps. +// The previous filter type-asserted the Topology* structs instead +// ([]TopRepeater, map[string]*ObserverReach, ...), every assertion missed, and +// blacklisted nodes were returned unfiltered. This filter works on the shape +// the store actually produces, accepts the other shapes the same data can take +// (JSON-decoded []interface{}, the typed structs), and fails closed on +// anything else: a part or entry it cannot read is dropped, never passed on. +// +// The store hands the handler its shared cached result — the recomputer +// snapshot or a topoCache entry — so nothing here writes to its input. Parts +// are rebuilt only when something in them is hidden; with nothing to hide the +// input is returned as is. + +// topologyListParts are the list-valued parts that carry node pubkeys. +// perObserverReach (an object of observers → rings → nodes) is handled +// separately. +var topologyListParts = []string{"topRepeaters", "topPairs", "bestPathList", "multiObsNodes"} + +// filterBlacklistedFromTopology returns data with every entry that refers to a +// blacklisted node (Config.IsBlacklisted: trimmed, case-insensitive) or to a +// name hidden by HiddenNamePrefixes (#1181) removed from topRepeaters, +// topPairs (either side), bestPathList, multiObsNodes and +// perObserverReach{}.rings[].nodes[]. The input is never modified. +func (s *Server) filterBlacklistedFromTopology(data map[string]interface{}) map[string]interface{} { + if data == nil || s == nil || s.cfg == nil { + return data + } + var out map[string]interface{} // copy-on-write top level + set := func(key string, v interface{}) { + if out == nil { + out = make(map[string]interface{}, len(data)) + for k, val := range data { + out[k] = val + } + } + out[key] = v + } + for _, key := range topologyListParts { + v, ok := data[key] + if !ok { + continue + } + hidden := s.topologyNodeHidden + switch key { + case "topPairs": + hidden = s.topologyPairHidden + case "bestPathList": + hidden = s.topologyBestPathHidden + } + if nv, changed := filterTopologyList(key, v, hidden); changed { + set(key, nv) + } + } + if v, ok := data["perObserverReach"]; ok { + if nv, changed := s.filterTopologyReach(v); changed { + set("perObserverReach", nv) + } + } + if out == nil { + return data + } + return out +} + +// topologyPubkeyHidden: a string pubkey is checked against the blacklist; an +// absent (nil) one is an unresolved hop and has nothing to check; anything +// else cannot be checked and is treated as hidden. +func (s *Server) topologyPubkeyHidden(v interface{}) bool { + switch pk := v.(type) { + case nil: + return false + case string: + return s.cfg.IsBlacklisted(pk) + default: + return true + } +} + +// topologyNameHidden applies HiddenNamePrefixes; a name of an unexpected type +// cannot be checked and is treated as hidden. +func (s *Server) topologyNameHidden(v interface{}) bool { + switch name := v.(type) { + case nil: + return false + case string: + return s.cfg.IsNameHidden(name) + default: + return true + } +} + +// topRepeaters, multiObsNodes and perObserverReach nodes. +func (s *Server) topologyNodeHidden(e map[string]interface{}) bool { + return s.topologyPubkeyHidden(e["pubkey"]) || s.topologyNameHidden(e["name"]) +} + +// topPairs: the pair goes if either side is hidden. +func (s *Server) topologyPairHidden(e map[string]interface{}) bool { + return s.topologyPubkeyHidden(e["pubkeyA"]) || s.topologyPubkeyHidden(e["pubkeyB"]) || + s.topologyNameHidden(e["nameA"]) || s.topologyNameHidden(e["nameB"]) +} + +// bestPathList: the entry's own pubkey and name, plus — as this part always +// did — the node's stored name (isPubkeyHidden). +func (s *Server) topologyBestPathHidden(e map[string]interface{}) bool { + if s.topologyPubkeyHidden(e["pubkey"]) || s.topologyNameHidden(e["name"]) { + return true + } + pk, _ := e["pubkey"].(string) + return pk != "" && s.isPubkeyHidden(pk) +} + +// topologyEntries reads a list part as its entries. ok is false when v is not +// a list of objects. native is true when v is already []map[string]interface{} +// (the store's shape) or nil; other readable shapes are converted, the typed +// structs via a JSON round trip. +func topologyEntries(v interface{}) (entries []map[string]interface{}, native, ok bool) { + switch l := v.(type) { + case nil: + return nil, true, true + case []map[string]interface{}: + return l, true, true + case []interface{}: + entries = make([]map[string]interface{}, 0, len(l)) + for _, e := range l { + m, isMap := e.(map[string]interface{}) + if !isMap { + return nil, false, false + } + entries = append(entries, m) + } + return entries, false, true + } + b, err := json.Marshal(v) + if err != nil || json.Unmarshal(b, &entries) != nil { + return nil, false, false + } + for _, e := range entries { + if e == nil { + return nil, false, false + } + } + return entries, false, true +} + +// filterTopologyList returns the part without hidden entries. changed is false +// (and v is returned as is) when there is nothing to remove. A part that is not +// a list of objects is replaced by an empty list — fail closed. +func filterTopologyList(key string, v interface{}, hidden func(map[string]interface{}) bool) (interface{}, bool) { + entries, _, ok := topologyEntries(v) + if !ok { + log.Printf("[privacy] topology: %s has an unexpected shape (%T); dropped", key, v) + return []map[string]interface{}{}, true + } + first := -1 + for i, e := range entries { + if hidden(e) { + first = i + break + } + } + if first < 0 { + return v, false + } + kept := make([]map[string]interface{}, first, len(entries)-1) + copy(kept, entries[:first]) + for _, e := range entries[first+1:] { + if !hidden(e) { + kept = append(kept, e) + } + } + return kept, true +} + +// filterTopologyReach filters perObserverReach: observer id → {observer_name, +// rings: [{hops, nodes: [...]}]}. Observers, rings and nodes are copied only +// where something changes; anything unreadable is dropped. +func (s *Server) filterTopologyReach(v interface{}) (interface{}, bool) { + var observers map[string]interface{} + converted := false + switch m := v.(type) { + case nil: + return v, false + case map[string]interface{}: + observers = m + default: // map[string]*ObserverReach or an unknown shape + b, err := json.Marshal(v) + if err != nil || json.Unmarshal(b, &observers) != nil { + log.Printf("[privacy] topology: perObserverReach has an unexpected shape (%T); dropped", v) + return map[string]interface{}{}, true + } + converted = true + } + changed := converted + out := make(map[string]interface{}, len(observers)) + for id, ov := range observers { + obs, ok := ov.(map[string]interface{}) + if !ok { + changed = true + continue + } + rings, ringsChanged, ok := s.filterReachRings(obs["rings"]) + if !ok { + log.Printf("[privacy] topology: perObserverReach[%s].rings has an unexpected shape; dropped", id) + changed = true + continue + } + if !ringsChanged { + out[id] = obs + continue + } + cp := make(map[string]interface{}, len(obs)) + for k, val := range obs { + cp[k] = val + } + cp["rings"] = rings + out[id] = cp + changed = true + } + if !changed { + return v, false + } + return out, true +} + +func (s *Server) filterReachRings(v interface{}) (rings interface{}, changed, ok bool) { + entries, native, ok := topologyEntries(v) + if !ok { + return nil, false, false + } + out := make([]map[string]interface{}, len(entries)) + for i, ring := range entries { + nodes, nodesChanged := filterTopologyList("perObserverReach nodes", ring["nodes"], s.topologyNodeHidden) + if !nodesChanged { + out[i] = ring + continue + } + cp := make(map[string]interface{}, len(ring)) + for k, val := range ring { + cp[k] = val + } + cp["nodes"] = nodes + out[i] = cp + changed = true + } + if !changed && native { + return v, false, true + } + return out, changed || !native, true +} From 342864b7e6516114f8d72f715a59d3327a53067a Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Thu, 24 Sep 2026 17:22:12 +0200 Subject: [PATCH 3/5] perf(server): no allocations in the topology filter when nothing is hidden (#91) With a blacklist configured but none of its nodes in the topology, the filter allocated on every request: - perObserverReach always built a new observer map and ring slice; - json.Unmarshal into the named result / outer variable moved those to the heap on every call, the native path included; - bound method values passed as predicates escaped. Copy-on-write for observers and rings, local unmarshal targets confined to the conversion branches, and method expressions for the predicates. Measured on the seeded store: 0 B / 0 allocs per call without a match (~58 ns per entry, dominated by Config.IsBlacklisted's normalisation); warm endpoint allocations equal to master. Co-Authored-By: Claude Opus 5.5 --- cmd/server/topology_privacy.go | 79 +++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 25 deletions(-) diff --git a/cmd/server/topology_privacy.go b/cmd/server/topology_privacy.go index 7329ae0e5..e5cc3cb91 100644 --- a/cmd/server/topology_privacy.go +++ b/cmd/server/topology_privacy.go @@ -49,14 +49,14 @@ func (s *Server) filterBlacklistedFromTopology(data map[string]interface{}) map[ if !ok { continue } - hidden := s.topologyNodeHidden + hidden := (*Server).topologyNodeHidden switch key { case "topPairs": - hidden = s.topologyPairHidden + hidden = (*Server).topologyPairHidden case "bestPathList": - hidden = s.topologyBestPathHidden + hidden = (*Server).topologyBestPathHidden } - if nv, changed := filterTopologyList(key, v, hidden); changed { + if nv, changed := s.filterTopologyList(key, v, hidden); changed { set(key, nv) } } @@ -140,22 +140,28 @@ func topologyEntries(v interface{}) (entries []map[string]interface{}, native, o } return entries, false, true } + // A local target, not the named result: taking the result's address + // would move it to the heap on every call, the native path included. + var converted []map[string]interface{} b, err := json.Marshal(v) - if err != nil || json.Unmarshal(b, &entries) != nil { + if err != nil || json.Unmarshal(b, &converted) != nil { return nil, false, false } - for _, e := range entries { + for _, e := range converted { if e == nil { return nil, false, false } } - return entries, false, true + return converted, false, true } // filterTopologyList returns the part without hidden entries. changed is false // (and v is returned as is) when there is nothing to remove. A part that is not // a list of objects is replaced by an empty list — fail closed. -func filterTopologyList(key string, v interface{}, hidden func(map[string]interface{}) bool) (interface{}, bool) { +// +// hidden is a method expression rather than a bound method value, so passing +// it does not allocate on the per-request path. +func (s *Server) filterTopologyList(key string, v interface{}, hidden func(*Server, map[string]interface{}) bool) (interface{}, bool) { entries, _, ok := topologyEntries(v) if !ok { log.Printf("[privacy] topology: %s has an unexpected shape (%T); dropped", key, v) @@ -163,7 +169,7 @@ func filterTopologyList(key string, v interface{}, hidden func(map[string]interf } first := -1 for i, e := range entries { - if hidden(e) { + if hidden(s, e) { first = i break } @@ -174,7 +180,7 @@ func filterTopologyList(key string, v interface{}, hidden func(map[string]interf kept := make([]map[string]interface{}, first, len(entries)-1) copy(kept, entries[:first]) for _, e := range entries[first+1:] { - if !hidden(e) { + if !hidden(s, e) { kept = append(kept, e) } } @@ -183,7 +189,8 @@ func filterTopologyList(key string, v interface{}, hidden func(map[string]interf // filterTopologyReach filters perObserverReach: observer id → {observer_name, // rings: [{hops, nodes: [...]}]}. Observers, rings and nodes are copied only -// where something changes; anything unreadable is dropped. +// where something changes (nothing is allocated when nothing is hidden); +// anything unreadable is dropped. func (s *Server) filterTopologyReach(v interface{}) (interface{}, bool) { var observers map[string]interface{} converted := false @@ -193,29 +200,42 @@ func (s *Server) filterTopologyReach(v interface{}) (interface{}, bool) { case map[string]interface{}: observers = m default: // map[string]*ObserverReach or an unknown shape + var decoded map[string]interface{} // local: see topologyEntries b, err := json.Marshal(v) - if err != nil || json.Unmarshal(b, &observers) != nil { + if err != nil || json.Unmarshal(b, &decoded) != nil { log.Printf("[privacy] topology: perObserverReach has an unexpected shape (%T); dropped", v) return map[string]interface{}{}, true } + observers = decoded converted = true } - changed := converted - out := make(map[string]interface{}, len(observers)) + var out map[string]interface{} // copy-on-write + touch := func() { + if out == nil { + out = make(map[string]interface{}, len(observers)) + for k, val := range observers { + out[k] = val + } + } + } + if converted { + touch() + } for id, ov := range observers { obs, ok := ov.(map[string]interface{}) if !ok { - changed = true + touch() + delete(out, id) continue } rings, ringsChanged, ok := s.filterReachRings(obs["rings"]) if !ok { log.Printf("[privacy] topology: perObserverReach[%s].rings has an unexpected shape; dropped", id) - changed = true + touch() + delete(out, id) continue } if !ringsChanged { - out[id] = obs continue } cp := make(map[string]interface{}, len(obs)) @@ -223,37 +243,46 @@ func (s *Server) filterTopologyReach(v interface{}) (interface{}, bool) { cp[k] = val } cp["rings"] = rings + touch() out[id] = cp - changed = true } - if !changed { + if out == nil { return v, false } return out, true } +// filterReachRings filters the nodes of each ring; rings are copied only when +// their nodes change, and a non-native (converted) ring list is always +// returned as the converted copy. func (s *Server) filterReachRings(v interface{}) (rings interface{}, changed, ok bool) { entries, native, ok := topologyEntries(v) if !ok { return nil, false, false } - out := make([]map[string]interface{}, len(entries)) + var out []map[string]interface{} // copy-on-write for i, ring := range entries { - nodes, nodesChanged := filterTopologyList("perObserverReach nodes", ring["nodes"], s.topologyNodeHidden) + nodes, nodesChanged := s.filterTopologyList("perObserverReach nodes", ring["nodes"], (*Server).topologyNodeHidden) if !nodesChanged { - out[i] = ring continue } + if out == nil { + out = make([]map[string]interface{}, len(entries)) + copy(out, entries) + } cp := make(map[string]interface{}, len(ring)) for k, val := range ring { cp[k] = val } cp["nodes"] = nodes out[i] = cp - changed = true } - if !changed && native { + switch { + case out != nil: + return out, true, true + case !native: + return entries, true, true + default: return v, false, true } - return out, changed || !native, true } From 2adf1ae8fc8e8e57f111ea9120b6666840afef4a Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Thu, 24 Sep 2026 17:37:11 +0200 Subject: [PATCH 4/5] test(server): topology filter without DB lookups, unresolved hops, odd names (#91) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review of the fix (no blockers) found gaps: - with HiddenNamePrefixes set, bestPathList ran isPubkeyHidden — one SQLite query per entry, up to 50 per request; a server without a database now makes any per-entry lookup panic (red on 342864b7); - nothing proved unresolved hops (pubkey nil) are kept; - nothing proved an entry whose name is not a string is dropped. Co-Authored-By: Claude Opus 5.5 --- cmd/server/topology_blacklist_filter_test.go | 58 ++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/cmd/server/topology_blacklist_filter_test.go b/cmd/server/topology_blacklist_filter_test.go index 3ce43b079..a8a58b73e 100644 --- a/cmd/server/topology_blacklist_filter_test.go +++ b/cmd/server/topology_blacklist_filter_test.go @@ -656,3 +656,61 @@ func TestFilterBlacklistedFromTopology_UnknownShapeFailsClosed(t *testing.T) { }) } } + +// The filter decides from the response alone. It must not look nodes up in +// the database per entry: with HiddenNamePrefixes set that was one SQLite +// query per bestPathList entry (up to 50) on every request. A server without +// a database makes any such lookup panic. +func TestFilterBlacklistedFromTopology_NoDatabaseLookups(t *testing.T) { + _, data := computedTopology(t) + srv := &Server{cfg: &Config{}} + srv.cfg.SetHiddenNamePrefixes([]string{"Targ"}) + out := jsonOf(t, srv.filterBlacklistedFromTopology(data)) + if strings.Contains(out, topoTarget) { + t.Errorf("name-hidden target still present") + } + if !strings.Contains(out, topoOther) { + t.Errorf("visible nodes were dropped") + } +} + +// Unresolved hops carry no pubkey (nil): nothing to match, so they stay. +func TestFilterBlacklistedFromTopology_UnresolvedHopsKept(t *testing.T) { + srv, _ := computedTopology(t) + unresolved := func(hop string) map[string]interface{} { + return map[string]interface{}{"hop": hop, "name": nil, "pubkey": nil, "count": 1} + } + data := map[string]interface{}{ + "topRepeaters": []map[string]interface{}{unresolved("ee01"), {"hop": "7a", "name": "Target", "pubkey": topoTarget}}, + "perObserverReach": map[string]interface{}{"obs1": map[string]interface{}{"observer_name": "o", "rings": []map[string]interface{}{ + {"hops": 1, "nodes": []map[string]interface{}{unresolved("ee02"), {"hop": "7a", "pubkey": topoTarget}}}}}}, + } + out := jsonOf(t, srv.filterBlacklistedFromTopology(data)) + if strings.Contains(out, topoTarget) { + t.Errorf("target kept: %s", out) + } + for _, hop := range []string{"ee01", "ee02"} { + if !strings.Contains(out, hop) { + t.Errorf("unresolved hop %s dropped: %s", hop, out) + } + } +} + +// A name the filter cannot read (not a string) cannot be checked against +// HiddenNamePrefixes, so the entry goes — fail closed, like pubkeys. +func TestFilterBlacklistedFromTopology_NonStringNameFailsClosed(t *testing.T) { + srv, _ := computedTopology(t) + data := map[string]interface{}{ + "topRepeaters": []map[string]interface{}{ + {"hop": "9d", "pubkey": topoOther, "name": map[string]interface{}{"n": "odd-name-marker"}}, + {"hop": "5b", "pubkey": topoLower, "name": "Lower"}, + }, + } + out := jsonOf(t, srv.filterBlacklistedFromTopology(data)) + if strings.Contains(out, "odd-name-marker") { + t.Errorf("entry with an unreadable name kept: %s", out) + } + if !strings.Contains(out, topoLower) { + t.Errorf("readable entry dropped: %s", out) + } +} From 90cff688ad41ec1b10024b05474b666a607e2c35 Mon Sep 17 00:00:00 2001 From: Dennis Jakobsen Date: Thu, 24 Sep 2026 17:37:11 +0200 Subject: [PATCH 5/5] reviewfix(server): no per-entry database lookup in the topology filter (#91) bestPathList used isPubkeyHidden, a GetNodeByPubkey query per entry when HiddenNamePrefixes is configured. The entry already carries the node's resolved name, so bestPathList now uses the same predicate as the other parts (pubkey blacklisted or name hidden) and the filter decides from the response alone. Co-Authored-By: Claude Opus 5.5 --- cmd/server/topology_privacy.go | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/cmd/server/topology_privacy.go b/cmd/server/topology_privacy.go index e5cc3cb91..391e2d8b6 100644 --- a/cmd/server/topology_privacy.go +++ b/cmd/server/topology_privacy.go @@ -50,11 +50,8 @@ func (s *Server) filterBlacklistedFromTopology(data map[string]interface{}) map[ continue } hidden := (*Server).topologyNodeHidden - switch key { - case "topPairs": + if key == "topPairs" { hidden = (*Server).topologyPairHidden - case "bestPathList": - hidden = (*Server).topologyBestPathHidden } if nv, changed := s.filterTopologyList(key, v, hidden); changed { set(key, nv) @@ -98,7 +95,10 @@ func (s *Server) topologyNameHidden(v interface{}) bool { } } -// topRepeaters, multiObsNodes and perObserverReach nodes. +// topRepeaters, bestPathList, multiObsNodes and perObserverReach nodes. The +// entry carries the node's resolved name, so nothing is looked up per entry +// (an isPubkeyHidden database query per bestPathList entry was up to 50 +// queries a request). func (s *Server) topologyNodeHidden(e map[string]interface{}) bool { return s.topologyPubkeyHidden(e["pubkey"]) || s.topologyNameHidden(e["name"]) } @@ -109,16 +109,6 @@ func (s *Server) topologyPairHidden(e map[string]interface{}) bool { s.topologyNameHidden(e["nameA"]) || s.topologyNameHidden(e["nameB"]) } -// bestPathList: the entry's own pubkey and name, plus — as this part always -// did — the node's stored name (isPubkeyHidden). -func (s *Server) topologyBestPathHidden(e map[string]interface{}) bool { - if s.topologyPubkeyHidden(e["pubkey"]) || s.topologyNameHidden(e["name"]) { - return true - } - pk, _ := e["pubkey"].(string) - return pk != "" && s.isPubkeyHidden(pk) -} - // topologyEntries reads a list part as its entries. ok is false when v is not // a list of objects. native is true when v is already []map[string]interface{} // (the store's shape) or nil; other readable shapes are converted, the typed