From d9aebf472d65889dca5c68646083fd63fca24a59 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 9 Sep 2026 07:23:40 +0000 Subject: [PATCH] state: exclude self from peers on the named peer-ID path ListPeers with explicit IDs filtered every node, not every peer, so a change batch naming the recipient returned it as its own peer. db.ListPeers keeps this out with `id <> nodeID`; the NodeStore rewrite dropped it. --- hscontrol/state/node_store_test.go | 50 ++++++++++++++++++++++++++++++ hscontrol/state/state.go | 11 +++++++ 2 files changed, 61 insertions(+) diff --git a/hscontrol/state/node_store_test.go b/hscontrol/state/node_store_test.go index 9d605ca6e..1fd04c385 100644 --- a/hscontrol/state/node_store_test.go +++ b/hscontrol/state/node_store_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -1361,3 +1362,52 @@ func TestGetNodesByMachineKeyAllUsers(t *testing.T) { require.Equal(t, types.NodeID(3), all[types.UserID(0)].ID()) }) } + +// TestListPeersExcludesSelf proves a node is never returned among its own +// peers, on both the snapshot path and the explicit peer-ID path. +// +// The explicit path is reached for incremental updates, where the caller +// passes the IDs named by a change batch — a batch that may include the +// recipient. Without the exclusion the recipient reaches the mapper as one of +// its own peers, is emitted in [tailcfg.MapResponse.PeersChanged], and the +// Tailscale client merges it into its peer map next to the self node. +func TestListPeersExcludesSelf(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("peer-user") + nodes := database.CreateRegisteredNodesForTest(user, 3, "peer-node") + require.NoError(t, database.Close()) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + allIDs := make([]types.NodeID, 0, len(nodes)) + for _, n := range nodes { + allIDs = append(allIDs, n.ID) + } + + for _, self := range allIDs { + t.Run(self.String(), func(t *testing.T) { + snapshot := s.ListPeers(self) + for _, peer := range snapshot.All() { + require.NotEqual(t, self, peer.ID(), "node listed in its own snapshot peers") + } + + // Every node named, the recipient included. + named := s.ListPeers(self, allIDs...) + require.Equal(t, len(allIDs)-1, named.Len(), "self must be dropped, every other named node kept") + + for _, peer := range named.All() { + require.NotEqual(t, self, peer.ID(), "node listed in its own named peers") + } + + // Naming only the recipient yields nothing. + require.Zero(t, s.ListPeers(self, self).Len(), "naming only self must yield no peers") + }) + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 00703893d..a6f94d00d 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -883,6 +883,17 @@ func (s *State) ListPeers(nodeID types.NodeID, peerIDs ...types.NodeID) views.Sl var filteredNodes []types.NodeView for _, node := range allNodes.All() { + // A node is never its own peer. [db.ListPeers] enforces this with + // `id <> nodeID`; the caller may name the recipient in peerIDs + // (a change batch that includes it), and the mapper's only other + // self filter is [policy.ReduceNodes], which is skipped when the + // node has no matchers. Self would then reach the client in + // [tailcfg.MapResponse.PeersChanged], where it is merged into the + // peer map and listed alongside the self node. + if node.ID() == nodeID { + continue + } + if _, exists := nodeIDSet[node.ID()]; exists { filteredNodes = append(filteredNodes, node) }