Fix: filter path-hop candidates by resolved_path to prevent prefix collisions (#658)

## Problem The "Paths Through This Node" API endpoint (`/api/nodes/{pubkey}/paths`) returns unrelated packets when two nodes share a hex prefix. For example, querying paths for "Kpa Roof Solar" (`c0dedad4...`) returns 316 packets that actually belong to "C0ffee SF" (`C0FFEEC7...`) because both share the `c0` prefix in the `byPathHop` index. Fixes #655 ## Root Cause `handleNodePaths()` in `routes.go` collects candidates from the `byPathHop` index using 2-char and 4-char hex prefixes for speed, but never verifies that the target node actually appears in each candidate's resolved path. The broad index lookup is intentional, but the **post-filter was missing**. ## Fix Added `nodeInResolvedPath()` helper in `store.go` that checks whether a transmission's `resolved_path` (from the neighbor affinity graph via `resolveWithContext`) contains the target node's full pubkey. The filter: - **Includes** packets where `resolved_path` contains the target node's full pubkey - **Excludes** packets where `resolved_path` resolved to a different node (prefix collision) - **Excludes** packets where `resolved_path` is nil/empty (ambiguous — avoids false positives) The check examines both the best observation's resolved_path (`tx.ResolvedPath`) and all individual observations, so packets are included if *any* observation resolved the target. ## Tests - `TestNodeInResolvedPath` — unit test for the helper with 5 cases (match, different node, nil, all-nil elements, match in observation only) - `TestNodePathsPrefixCollisionFilter` — integration test: two nodes sharing `aa` prefix, verifies the collision packet is excluded from one and included for the other - Updated test DB schema to include `resolved_path` column and seed data with resolved pubkeys - All existing tests pass (165 additions, 8 modifications) ## Performance No impact on hot paths. The filter runs once per API call on the already-collected candidate set (typically small). `nodeInResolvedPath` is O(observations × hops) per candidate — negligible since observations per transmission are typically 1–5. --------- Co-authored-by: you <you@example.com>
2026-05-25 20:14:02 +00:00 · 2026-04-07 21:24:00 -07:00
parent b8e9b04a97
commit 22bf33700e
4 changed files with 172 additions and 8 deletions
@@ -72,7 +72,8 @@ func setupTestDB(t *testing.T) *DB {
 			rssi REAL,
 			score INTEGER,
 			path_json TEXT,
-			timestamp INTEGER NOT NULL
+			timestamp INTEGER NOT NULL,
+			resolved_path TEXT
 		);

 		CREATE TABLE IF NOT EXISTS observer_metrics (
@@ -95,7 +96,7 @@ func setupTestDB(t *testing.T) *DB {
 		t.Fatal(err)
 	}

-	return &DB{conn: conn, isV3: true}
+	return &DB{conn: conn, isV3: true, hasResolvedPath: true}
 }

 func seedTestData(t *testing.T, db *DB) {
@@ -132,14 +133,15 @@ func seedTestData(t *testing.T, db *DB) {
 		VALUES ('AA1F', 'def456abc1230099', ?, 1, 4, '{"pubKey":"aabbccdd11223344","name":"TestRepeater","type":"ADVERT","timestamp":1700000100,"timestampISO":"2023-11-14T22:14:40.000Z","signature":"fedcba","flags":{"isRepeater":true},"lat":37.5,"lon":-122.0}')`, yesterday)

 	// Seed observations (use unix timestamps)
-	db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp)
-		VALUES (1, 1, 12.5, -90, '["aa","bb"]', ?)`, recentEpoch)
-	db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp)
-		VALUES (1, 2, 8.0, -95, '["aa"]', ?)`, recentEpoch-100)
+	// resolved_path contains full pubkeys parallel to path_json hops
+	db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp, resolved_path)
+		VALUES (1, 1, 12.5, -90, '["aa","bb"]', ?, '["aabbccdd11223344","eeff00112233aabb"]')`, recentEpoch)
+	db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp, resolved_path)
+		VALUES (1, 2, 8.0, -95, '["aa"]', ?, '["aabbccdd11223344"]')`, recentEpoch-100)
 	db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp)
 		VALUES (2, 1, 15.0, -85, '[]', ?)`, yesterdayEpoch)
-	db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp)
-		VALUES (3, 1, 10.0, -92, '["cc"]', ?)`, yesterdayEpoch)
+	db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp, resolved_path)
+		VALUES (3, 1, 10.0, -92, '["cc"]', ?, '["1122334455667788"]')`, yesterdayEpoch)
 }

 func TestGetStats(t *testing.T) {
@@ -1176,6 +1176,17 @@ func (s *Server) handleNodePaths(w http.ResponseWriter, r *http.Request) {
 		}
 	}

+	// Post-filter: verify target node actually appears in each candidate's resolved_path.
+	// The byPathHop index uses short prefixes which can collide (e.g. "c0" matches multiple nodes).
+	// We lean on resolved_path (from neighbor affinity graph) to disambiguate.
+	filtered := candidates[:0] // reuse backing array
+	for _, tx := range candidates {
+		if nodeInResolvedPath(tx, lowerPK) {
+			filtered = append(filtered, tx)
+		}
+	}
+	candidates = filtered
+
 	type pathAgg struct {
 		Hops       []PathHopResp
 		Count      int
@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"strconv"
+	"strings"
 	"testing"
 	"time"

@@ -3538,6 +3539,122 @@ func TestNodePathsEndpointUsesIndex(t *testing.T) {
 	}
 }

+func TestNodePathsPrefixCollisionFilter(t *testing.T) {
+	// Two nodes share the "aa" prefix: TestRepeater (aabbccdd11223344) and a
+	// second node (aacafe0000000000). Packets whose resolved_path points to
+	// the second node must NOT appear when querying TestRepeater's paths.
+	srv, router := setupTestServer(t)
+
+	// Manually inject a transmission whose raw path contains "aa" but whose
+	// resolved_path points to the other node (aacafe0000000000).
+	now := time.Now().UTC()
+	recent := now.Add(-30 * time.Minute).Format(time.RFC3339)
+	recentEpoch := now.Add(-30 * time.Minute).Unix()
+
+	// Insert a second node with the same 2-char prefix
+	srv.db.conn.Exec(`INSERT OR IGNORE INTO nodes (public_key, name, role, last_seen, first_seen, advert_count)
+		VALUES ('aacafe0000000000', 'CollisionNode', 'repeater', ?, '2026-01-01T00:00:00Z', 5)`, recent)
+
+	// Insert a transmission with path hop "aa" that resolves to the OTHER node
+	srv.db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json)
+		VALUES ('FF01', 'collision_test_hash', ?, 1, 4, '{}')`, recent)
+	// Get its ID
+	var collisionTxID int
+	srv.db.conn.QueryRow(`SELECT id FROM transmissions WHERE hash='collision_test_hash'`).Scan(&collisionTxID)
+
+	srv.db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp, resolved_path)
+		VALUES (?, 1, 10.0, -90, '["aa","bb"]', ?, '["aacafe0000000000","eeff00112233aabb"]')`,
+		collisionTxID, recentEpoch)
+
+	// Reload store to pick up new data
+	store := NewPacketStore(srv.db, nil)
+	if err := store.Load(); err != nil {
+		t.Fatalf("store.Load failed: %v", err)
+	}
+	srv.store = store
+
+	// Query paths for TestRepeater — should NOT include the collision packet
+	req := httptest.NewRequest("GET", "/api/nodes/aabbccdd11223344/paths", nil)
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	if w.Code != 200 {
+		t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+
+	var resp struct {
+		Paths              []json.RawMessage `json:"paths"`
+		TotalTransmissions int               `json:"totalTransmissions"`
+	}
+	if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("bad JSON: %v", err)
+	}
+
+	// The collision packet should be filtered out. Only transmission 1 (and 3
+	// if prefix matches) should remain — but transmission 3 has path "cc" and
+	// resolved_path pointing to TestRoom, so only tx 1 should match.
+	// Check that collision_test_hash is not in any path group.
+	bodyStr := w.Body.String()
+	if strings.Contains(bodyStr, "collision_test_hash") {
+		t.Error("collision packet should have been filtered out but appeared in response")
+	}
+
+	// Query paths for CollisionNode — should include the collision packet
+	req2 := httptest.NewRequest("GET", "/api/nodes/aacafe0000000000/paths", nil)
+	w2 := httptest.NewRecorder()
+	router.ServeHTTP(w2, req2)
+
+	if w2.Code != 200 {
+		t.Fatalf("expected 200 for CollisionNode, got %d: %s", w2.Code, w2.Body.String())
+	}
+
+	body2 := w2.Body.String()
+	if !strings.Contains(body2, "collision_test_hash") {
+		t.Error("collision packet should appear for CollisionNode but was missing")
+	}
+}
+
+func TestNodeInResolvedPath(t *testing.T) {
+	target := "aabbccdd11223344"
+
+	// Case 1: tx.ResolvedPath contains target
+	pk := "aabbccdd11223344"
+	tx1 := &StoreTx{ResolvedPath: []*string{&pk}}
+	if !nodeInResolvedPath(tx1, target) {
+		t.Error("should match when ResolvedPath contains target")
+	}
+
+	// Case 2: tx.ResolvedPath contains different node
+	other := "aacafe0000000000"
+	tx2 := &StoreTx{ResolvedPath: []*string{&other}}
+	if nodeInResolvedPath(tx2, target) {
+		t.Error("should not match when ResolvedPath contains different node")
+	}
+
+	// Case 3: nil ResolvedPath — should match (no data to disambiguate, keep it)
+	tx3 := &StoreTx{}
+	if !nodeInResolvedPath(tx3, target) {
+		t.Error("should match when ResolvedPath is nil (no data to disambiguate)")
+	}
+
+	// Case 4: ResolvedPath with nil elements only — has data but no match
+	tx4 := &StoreTx{ResolvedPath: []*string{nil, nil}}
+	if nodeInResolvedPath(tx4, target) {
+		t.Error("should not match when all ResolvedPath elements are nil")
+	}
+
+	// Case 5: target in observation but not in tx.ResolvedPath
+	tx5 := &StoreTx{
+		ResolvedPath: []*string{&other},
+		Observations: []*StoreObs{
+			{ResolvedPath: []*string{&pk}},
+		},
+	}
+	if !nodeInResolvedPath(tx5, target) {
+		t.Error("should match when observation's ResolvedPath contains target")
+	}
+}
+
 func TestPathHopIndexIncrementalUpdate(t *testing.T) {
 	// Test that addTxToPathHopIndex and removeTxFromPathHopIndex work correctly
 	idx := make(map[string][]*StoreTx)
@@ -2157,6 +2157,40 @@ func resolvePayloadTypeName(pt *int) string {
 	return fmt.Sprintf("UNK(%d)", *pt)
 }

+// nodeInResolvedPath checks whether a transmission's resolved_path contains
+// the target node's full pubkey. Returns true if at least one observation's
+// resolved_path includes targetPK (lowercased). Excludes transmissions where
+// resolved_path is nil/empty or the hop resolved to a different node.
+func nodeInResolvedPath(tx *StoreTx, targetPK string) bool {
+	// If no resolved_path data exists anywhere on this tx, we can't
+	// disambiguate — return true to keep it (avoid dropping old data).
+	hasAny := false
+
+	// Check the best observation's resolved_path (stored on tx directly).
+	if tx.ResolvedPath != nil && len(tx.ResolvedPath) > 0 {
+		hasAny = true
+		for _, rp := range tx.ResolvedPath {
+			if rp != nil && strings.ToLower(*rp) == targetPK {
+				return true
+			}
+		}
+	}
+	// Also check all observations in case a non-best observation resolved it.
+	for _, obs := range tx.Observations {
+		if obs.ResolvedPath == nil || len(obs.ResolvedPath) == 0 {
+			continue
+		}
+		hasAny = true
+		for _, rp := range obs.ResolvedPath {
+			if rp != nil && strings.ToLower(*rp) == targetPK {
+				return true
+			}
+		}
+	}
+	// No resolved_path data at all — can't disambiguate, keep the candidate.
+	return !hasAny
+}
+
 // txGetParsedPath returns cached parsed path hops, parsing on first call.
 func txGetParsedPath(tx *StoreTx) []string {
 	if tx.pathParsed {