fix(analytics): apply nodeBlacklist and hiddenNamePrefixes to Route Patterns (#2184) (#2189)

Fixes #2184. ## Problem `nodeBlacklist` and `hiddenNamePrefixes` did not apply to Route Patterns: - **`subpaths` and `subpaths-bulk`:** `filterBlacklistedFromSubpaths` asserted `data["subpaths"].([]interface{})`, while the store builds `[]map[string]interface{}`, so it returned its input unchanged. It then read `hops` as a list, which is an `int` (the run length). It also ran only when `nodeBlacklist` was set, so `hiddenNamePrefixes` alone never filtered anything. - **`subpath-detail`:** the check compared each raw hop (a prefix) with full keys, so it matched only a client that sent a full key. - **Parent paths** in the detail response were not filtered. ## Change All in `cmd/server/routes.go`; `store.go` is untouched, so this doesn't overlap #2182. - **Lists:** an entry is dropped when a name in its resolved `path` is the name of a blacklisted node (names looked up in the store's node cache) or has a hidden prefix. A subpath carries names and prefixes, not keys, and the store picks a node per prefix from context, so re-resolving a prefix in the handler would not be guaranteed to land on the same node. Matching the names the reader would see avoids that. Trade-off: another node with exactly the same name as a blacklisted one is hidden too. - **Detail:** 404 when a hop resolved to a blacklisted node (by the resolved key in `nodes[].pubkey`, so a node without a name is caught and one that only shares its name is not) or to a hidden-prefix name; parent paths that show one are dropped (by name, like the lists). The existing raw-hop check stays. - The filter runs when either `nodeBlacklist` or `hiddenNamePrefixes` is set. - `dropHiddenPaths` replaces `filterBlacklistedFromSubpaths` and returns a copy: the input can be the store's cached response, which the old function mutated in place. - `totalPaths` and `pct` are unchanged, so percentages stay relative to every stored path, the filtered ones included. **Perf:** per request, one pass over the node cache (about 2k nodes on the deployment below) to collect blacklisted names, then at most 8 lookups per entry on at most `limit` entries. It runs outside the store lock and only when a filter is configured. `map[string]interface{}` count in `routes.go`: 3 removed, 3 added. ## Tests - `subpaths_blacklist_2184_test.go`: on the seeded path `["aa","bb"]` (first hop resolves to TestRepeater), for `subpaths`, `subpaths-bulk` and `subpath-detail`: - unfiltered, the repeater is listed (so the test is not vacuous); - a blacklisted key, also in capitals, removes it and the detail returns 404; - a hidden name prefix removes it; - a different key with the same 1-byte prefix leaves it listed; - clearing the blacklist afterwards shows it again (the cached response was not changed). - subpath-detail: a blacklisted node without a name returns 404, a different node that only shares its name does not hide the detail. - Red commits first (`0ebee485`, `4acfb70a`, failing on assertions), then green. - `cd cmd/server && go test ./...` passes (`TestSaveGeoFilterPreservesFileMode` skipped, a Windows-only failure on master too); `go test -race -run 2184` passes; `go vet` clean. ## Validation on a live-fed staging instance `nodeBlacklist` temporarily set to the key of the node in the top Route Patterns entry (5848 occurrences), then restored: | | blacklist set | restored | |---|---|---| | `subpaths?limit=200` entries showing the node | 0 (185 entries left) | 15 (200 entries) | | `subpaths-bulk` groups 2-2 / 3-3 / 5-8 showing it | 0 / 0 / 0 | | | `subpath-detail` on a path through it | 404 | | | `subpath-detail` on a path without it | 200 | | ## Not done - Other analytics responses that carry node names (for example topology) were not audited for the same gap. - The UI still shows `pct` relative to all paths; whether a filtered list should renormalise is left open. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-15 07:09:58 +00:00 · 2026-10-13 08:35:16 +02:00
parent 45680a22cf
commit abc6ab9b59
2 changed files with 268 additions and 30 deletions
@@ -7,10 +7,12 @@ import (
 	"errors"
 	"fmt"
 	"log"
+	"maps"
 	"math"
 	"net/http"
 	"regexp"
 	"runtime"
+	"slices"
 	"sort"
 	"strconv"
 	"strings"
@@ -2618,8 +2620,8 @@ func (s *Server) handleAnalyticsSubpaths(w http.ResponseWriter, r *http.Request)
 		// Issue #1217: honor the Time window filter on Route Patterns.
 		window := ParseTimeWindow(r)
 		data := s.store.GetAnalyticsSubpathsWithWindow(region, minLen, maxLen, limit, window)
-		if s.cfg != nil && len(s.cfg.NodeBlacklist) > 0 {
-			data = s.filterBlacklistedFromSubpaths(data)
+		if s.subpathFilterActive() {
+			data, _ = s.dropHiddenPaths(data, "subpaths", "path", "", s.blacklistedNodeNames())
 		}
 		writeJSON(w, data)
 		return
@@ -2684,9 +2686,10 @@ func (s *Server) handleAnalyticsSubpathsBulk(w http.ResponseWriter, r *http.Requ
 	}

 	results := s.store.GetAnalyticsSubpathsBulkWithWindow(region, groups, ParseTimeWindow(r))
-	if s.cfg != nil && len(s.cfg.NodeBlacklist) > 0 {
+	if s.subpathFilterActive() {
+		names := s.blacklistedNodeNames()
 		for i, r := range results {
-			results[i] = s.filterBlacklistedFromSubpaths(r)
+			results[i], _ = s.dropHiddenPaths(r, "subpaths", "path", "", names)
 		}
 	}
 	writeJSON(w, map[string]interface{}{"results": results})
@@ -2722,7 +2725,19 @@ func (s *Server) handleAnalyticsSubpathDetail(w http.ResponseWriter, r *http.Req
 			writeIndexLoading503(w)
 			return
 		}
-		writeJSON(w, s.store.GetSubpathDetail(rawHops))
+		detail := s.store.GetSubpathDetail(rawHops)
+		// #2184: the raw hops are prefixes, so the check above only catches a
+		// full key. Check the nodes the store resolved them to instead.
+		if s.subpathFilterActive() {
+			// Match each hop by its resolved key, so a blacklisted node without a
+			// name is caught and a node that only shares its name is not.
+			if _, dropped := s.dropHiddenPaths(detail, "nodes", "name", "pubkey", nil); dropped > 0 {
+				writeError(w, 404, "Not found")
+				return
+			}
+			detail, _ = s.dropHiddenPaths(detail, "parentPaths", "path", "", s.blacklistedNodeNames())
+		}
+		writeJSON(w, detail)
 		return
 	}
 	writeJSON(w, SubpathDetailResponse{
@@ -3827,33 +3842,62 @@ func (s *Server) filterBlacklistedFromTopology(data map[string]interface{}) map[
 	return data
 }

-// filterBlacklistedFromSubpaths removes blacklisted node references from
-// the subpaths analytics response.
-func (s *Server) filterBlacklistedFromSubpaths(data map[string]interface{}) map[string]interface{} {
-	if subpaths, ok := data["subpaths"]; ok {
-		if arr, ok := subpaths.([]interface{}); ok {
-			var filtered []interface{}
-			for _, item := range arr {
-				if m, ok := item.(map[string]interface{}); ok {
-					if hops, ok := m["hops"].([]interface{}); ok {
-						skip := false
-						for _, h := range hops {
-							if hp, ok := h.(string); ok && (s.cfg.IsBlacklisted(hp) || s.isPubkeyHidden(hp)) {
-								skip = true
-								break
-							}
-						}
-						if skip {
-							continue
-						}
-					}
-				}
-				filtered = append(filtered, item)
-			}
-			data["subpaths"] = filtered
+// subpathFilterActive reports whether Route Patterns responses need the
+// nodeBlacklist / hiddenNamePrefixes filter.
+func (s *Server) subpathFilterActive() bool {
+	return s.cfg != nil && (len(s.cfg.NodeBlacklist) > 0 || len(s.cfg.HiddenNamePrefixes) > 0)
+}
+
+// blacklistedNodeNames returns the names of the blacklisted nodes the store
+// knows, for filtering responses that carry node names but no keys.
+func (s *Server) blacklistedNodeNames() map[string]bool {
+	if s.store == nil || s.cfg == nil || len(s.cfg.NodeBlacklist) == 0 {
+		return nil
+	}
+	nodes, _ := s.store.getCachedNodesAndPM()
+	names := make(map[string]bool)
+	for _, n := range nodes {
+		if n.Name != "" && s.cfg.IsBlacklisted(n.PublicKey) {
+			names[n.Name] = true
 		}
 	}
-	return data
+	return names
+}
+
+// subpathPathHidden reports whether a resolved " → " path shows a blacklisted
+// or hidden-prefix node. Issue #2184: a subpath carries the names the store
+// resolved its hops to, not keys, and a hop prefix alone cannot be pinned to
+// one node, so the filter matches what the reader would see.
+func (s *Server) subpathPathHidden(path string, blacklisted map[string]bool) bool {
+	for _, name := range strings.Split(path, " → ") {
+		if blacklisted[name] || s.cfg.IsNameHidden(name) {
+			return true
+		}
+	}
+	return false
+}
+
+// dropHiddenPaths removes the entries of data[listKey] whose field (a node
+// name, or a resolved " → " path) shows a blacklisted or hidden-prefix node,
+// or, when keyField is set, whose key there is blacklisted, and reports how
+// many it removed. It returns a copy: data may be the store's cached
+// response, shared with other requests.
+func (s *Server) dropHiddenPaths(data map[string]interface{}, listKey, field, keyField string, blacklisted map[string]bool) (map[string]interface{}, int) {
+	entries, ok := data[listKey].([]map[string]interface{})
+	if !ok {
+		return data, 0
+	}
+	kept := slices.Clone(entries)[:0]
+	for _, e := range entries {
+		path, _ := e[field].(string)
+		key, _ := e[keyField].(string)
+		if !s.subpathPathHidden(path, blacklisted) && (key == "" || !s.cfg.IsBlacklisted(key)) {
+			kept = append(kept, e)
+		}
+	}
+	out := maps.Clone(data)
+	out[listKey] = kept
+	return out, len(entries) - len(kept)
 }

 // handleDroppedPackets returns recently dropped packets for investigation.
@@ -0,0 +1,194 @@
+package main
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/gorilla/mux"
+)
+
+// Issue #2184: nodeBlacklist and hiddenNamePrefixes were not applied to Route
+// Patterns. The seeded path ["aa","bb"] resolves its first hop to TestRepeater
+// (aabbccdd11223344), so the subpath "TestRepeater → bb" is listed.
+
+const seededRepeaterKey = "aabbccdd11223344"
+
+func subpathPaths(t *testing.T, body []byte) []string {
+	t.Helper()
+	var resp struct {
+		Subpaths []struct {
+			Path string `json:"path"`
+		} `json:"subpaths"`
+	}
+	if err := json.Unmarshal(body, &resp); err != nil {
+		t.Fatalf("unmarshal: %v body=%s", err, body)
+	}
+	out := make([]string, 0, len(resp.Subpaths))
+	for _, sp := range resp.Subpaths {
+		out = append(out, sp.Path)
+	}
+	return out
+}
+
+func bulkSubpathPaths(t *testing.T, body []byte) []string {
+	t.Helper()
+	var resp struct {
+		Results []struct {
+			Subpaths []struct {
+				Path string `json:"path"`
+			} `json:"subpaths"`
+		} `json:"results"`
+	}
+	if err := json.Unmarshal(body, &resp); err != nil {
+		t.Fatalf("unmarshal: %v body=%s", err, body)
+	}
+	var out []string
+	for _, r := range resp.Results {
+		for _, sp := range r.Subpaths {
+			out = append(out, sp.Path)
+		}
+	}
+	return out
+}
+
+func anyContains(paths []string, name string) bool {
+	for _, p := range paths {
+		if strings.Contains(p, name) {
+			return true
+		}
+	}
+	return false
+}
+
+func getSubpathBody2184(t *testing.T, srv *Server, url string) (int, []byte) {
+	t.Helper()
+	req := httptest.NewRequest("GET", url, nil)
+	w := httptest.NewRecorder()
+	srv.router.ServeHTTP(w, req)
+	return w.Code, w.Body.Bytes()
+}
+
+func TestSubpathsBlacklist2184(t *testing.T) {
+	cases := []struct {
+		name      string
+		blacklist []string
+		hidden    []string
+		wantShown bool
+	}{
+		{"no filter lists the repeater", nil, nil, true},
+		{"blacklisted key drops it", []string{seededRepeaterKey}, nil, false},
+		{"blacklisted key in capitals drops it", []string{strings.ToUpper(seededRepeaterKey)}, nil, false},
+		{"hidden name prefix drops it", nil, []string{"TestRep"}, false},
+		// Another key with the same 1-byte prefix: the displayed node is not
+		// the blacklisted one, so the subpath stays.
+		{"same-prefix key keeps it", []string{"aa00000000000000"}, nil, true},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			srv, _ := setupTestServer(t)
+			if c.blacklist != nil {
+				srv.cfg.NodeBlacklist = c.blacklist
+				srv.cfg.SetNodeBlacklist(c.blacklist)
+			}
+			if c.hidden != nil {
+				srv.cfg.SetHiddenNamePrefixes(c.hidden)
+			}
+
+			code, body := getSubpathBody2184(t, srv, "/api/analytics/subpaths?minLen=2&maxLen=8")
+			if code != http.StatusOK {
+				t.Fatalf("subpaths status=%d body=%s", code, body)
+			}
+			if got := anyContains(subpathPaths(t, body), "TestRepeater"); got != c.wantShown {
+				t.Errorf("subpaths: TestRepeater shown=%v, want %v (%s)", got, c.wantShown, body)
+			}
+
+			code, body = getSubpathBody2184(t, srv, "/api/analytics/subpaths-bulk?groups=2-2:50,3-8:50")
+			if code != http.StatusOK {
+				t.Fatalf("subpaths-bulk status=%d body=%s", code, body)
+			}
+			if got := anyContains(bulkSubpathPaths(t, body), "TestRepeater"); got != c.wantShown {
+				t.Errorf("subpaths-bulk: TestRepeater shown=%v, want %v (%s)", got, c.wantShown, body)
+			}
+
+			code, body = getSubpathBody2184(t, srv, "/api/analytics/subpath-detail?hops=aa,bb")
+			wantCode := http.StatusOK
+			if !c.wantShown {
+				wantCode = http.StatusNotFound
+			}
+			if code != wantCode {
+				t.Errorf("subpath-detail status=%d, want %d (%s)", code, wantCode, body)
+			}
+		})
+	}
+}
+
+// The filter must not change the store's cached response: a later request,
+// or one after the blacklist is cleared, still sees the full list.
+func TestSubpathsBlacklistLeavesCacheIntact2184(t *testing.T) {
+	srv, _ := setupTestServer(t)
+	srv.cfg.NodeBlacklist = []string{seededRepeaterKey}
+	srv.cfg.SetNodeBlacklist(srv.cfg.NodeBlacklist)
+	if _, body := getSubpathBody2184(t, srv, "/api/analytics/subpaths?minLen=2&maxLen=8"); anyContains(subpathPaths(t, body), "TestRepeater") {
+		t.Fatalf("blacklisted repeater still listed: %s", body)
+	}
+
+	srv.cfg.NodeBlacklist = nil
+	srv.cfg.SetNodeBlacklist(nil)
+	if _, body := getSubpathBody2184(t, srv, "/api/analytics/subpaths?minLen=2&maxLen=8"); !anyContains(subpathPaths(t, body), "TestRepeater") {
+		t.Errorf("after clearing the blacklist the repeater is missing: the filter mutated the cached response (%s)", body)
+	}
+}
+
+// setupSubpathServer2184 is setupTestServer with a SQL statement run on the
+// seeded DB before the store loads, so the store sees the changed rows.
+func setupSubpathServer2184(t *testing.T, stmt string) *Server {
+	t.Helper()
+	db := setupTestDB(t)
+	seedTestData(t, db)
+	if _, err := db.conn.Exec(stmt); err != nil {
+		t.Fatalf("exec %q: %v", stmt, err)
+	}
+	srv := NewServer(db, &Config{Port: 3000}, NewHub())
+	store := NewPacketStore(db, nil)
+	if err := store.Load(); err != nil {
+		t.Fatalf("store.Load failed: %v", err)
+	}
+	if !store.WaitIndexesReady(5 * time.Second) {
+		t.Fatalf("background indexes never became ready")
+	}
+	srv.store = store
+	srv.router = mux.NewRouter()
+	srv.RegisterRoutes(srv.router)
+	return srv
+}
+
+// A blacklisted node without a name (the firmware allows one) has no name to
+// match, so the detail must check the resolved key.
+func TestSubpathDetailBlacklistUnnamedNode2184(t *testing.T) {
+	srv := setupSubpathServer2184(t, `UPDATE nodes SET name = '' WHERE public_key = '`+seededRepeaterKey+`'`)
+	srv.cfg.NodeBlacklist = []string{seededRepeaterKey}
+	srv.cfg.SetNodeBlacklist(srv.cfg.NodeBlacklist)
+
+	code, body := getSubpathBody2184(t, srv, "/api/analytics/subpath-detail?hops=aa,bb")
+	if code != http.StatusNotFound {
+		t.Errorf("subpath-detail status=%d, want 404 (%s)", code, body)
+	}
+}
+
+// Another node that only shares the blacklisted node's name does not hide the
+// detail of a path through the seeded repeater.
+func TestSubpathDetailBlacklistSameNameOtherNode2184(t *testing.T) {
+	const other = "ff99887766554433"
+	srv := setupSubpathServer2184(t, `INSERT INTO nodes (public_key, name, role) VALUES ('`+other+`', 'TestRepeater', 'repeater')`)
+	srv.cfg.NodeBlacklist = []string{other}
+	srv.cfg.SetNodeBlacklist(srv.cfg.NodeBlacklist)
+
+	code, body := getSubpathBody2184(t, srv, "/api/analytics/subpath-detail?hops=aa,bb")
+	if code != http.StatusOK {
+		t.Errorf("subpath-detail status=%d, want 200 (%s)", code, body)
+	}
+}