From baf2c035085abd1856efa4d237ba2a730c1c7bf4 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Tue, 24 Mar 2026 12:35:58 +0100 Subject: [PATCH] Fix CodeRabbit findings: hasIPv6Changed restart loop, empty peerIPs panic, v6 validation --- .github/workflows/wasm-build-validation.yml | 4 ++-- client/iface/wgaddr/address.go | 5 ++++- client/internal/engine.go | 25 ++++++++++----------- client/internal/engine_test.go | 16 ++++++++----- 4 files changed, 28 insertions(+), 22 deletions(-) diff --git a/.github/workflows/wasm-build-validation.yml b/.github/workflows/wasm-build-validation.yml index 47e45165b..81ae36e78 100644 --- a/.github/workflows/wasm-build-validation.yml +++ b/.github/workflows/wasm-build-validation.yml @@ -61,8 +61,8 @@ jobs: echo "Size: ${SIZE} bytes (${SIZE_MB} MB)" - if [ ${SIZE} -gt 57671680 ]; then - echo "Wasm binary size (${SIZE_MB}MB) exceeds 55MB limit!" + if [ ${SIZE} -gt 58720256 ]; then + echo "Wasm binary size (${SIZE_MB}MB) exceeds 56MB limit!" exit 1 fi diff --git a/client/iface/wgaddr/address.go b/client/iface/wgaddr/address.go index cc8afcf72..43d1ec9aa 100644 --- a/client/iface/wgaddr/address.go +++ b/client/iface/wgaddr/address.go @@ -71,6 +71,9 @@ func (addr *Address) SetIPv6FromCompact(raw []byte) error { if err != nil { return fmt.Errorf("decode v6 overlay address: %w", err) } + if !prefix.Addr().Is6() { + return fmt.Errorf("expected IPv6 address, got %s", prefix.Addr()) + } addr.IPv6 = prefix.Addr() addr.IPv6Net = prefix.Masked() return nil @@ -78,7 +81,7 @@ func (addr *Address) SetIPv6FromCompact(raw []byte) error { // ClearIPv6 removes the IPv6 overlay address, leaving only v4. // -//nolint:recvcheck // ClearIPv6 is the only mutating method on this otherwise value-type struct. +//nolint:recvcheck func (addr *Address) ClearIPv6() { addr.IPv6 = netip.Addr{} addr.IPv6Net = netip.Prefix{} diff --git a/client/internal/engine.go b/client/internal/engine.go index 1d27df158..2fc1617b4 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -1035,22 +1035,24 @@ func (e *Engine) updateConfig(conf *mgmProto.PeerConfig) error { } // hasIPv6Changed reports whether the IPv6 overlay address in the peer config -// differs from the current interface address (added, removed, or changed). +// differs from the configured address (added, removed, or changed). +// Compares against e.config.WgAddr (not the interface address, which may have +// been cleared by ClearIPv6 if OS assignment failed). func (e *Engine) hasIPv6Changed(conf *mgmProto.PeerConfig) bool { - current := e.wgInterface.Address() + current := e.config.WgAddr raw := conf.GetAddressV6() if len(raw) == 0 { return current.HasIPv6() } - addr, err := netiputil.DecodeAddr(raw) + prefix, err := netiputil.DecodePrefix(raw) if err != nil { log.Warnf("decode v6 overlay address: %v", err) return false } - return !current.HasIPv6() || current.IPv6 != addr + return !current.HasIPv6() || current.IPv6 != prefix.Addr() || current.IPv6Net != prefix.Masked() } func (e *Engine) receiveJobEvents() { @@ -1540,20 +1542,17 @@ func (e *Engine) addNewPeer(peerConfig *mgmProto.RemotePeerConfig) error { peerIPs = append(peerIPs, allowedNetIP) } + if len(peerIPs) == 0 { + return fmt.Errorf("peer %s has no usable AllowedIPs", peerKey) + } + conn, err := e.createPeerConn(peerKey, peerIPs, peerConfig.AgentVersion) if err != nil { return fmt.Errorf("create peer connection: %w", err) } - var peerIPv6 string - ourV6Net := e.wgInterface.Address().IPv6Net - for _, pip := range peerIPs { - if pip.Addr().Is6() && pip.Bits() == 128 && ourV6Net.Contains(pip.Addr()) { - peerIPv6 = pip.Addr().String() - break - } - } - err = e.statusRecorder.AddPeer(peerKey, peerConfig.Fqdn, peerIPs[0].Addr().String(), peerIPv6) + peerV4, peerV6 := splitAllowedIPs(peerConfig.GetAllowedIps(), e.wgInterface.Address().IPv6Net) + err = e.statusRecorder.AddPeer(peerKey, peerConfig.Fqdn, peerV4, peerV6) if err != nil { log.Warnf("error adding peer %s to status recorder, got error: %v", peerKey, err) } diff --git a/client/internal/engine_test.go b/client/internal/engine_test.go index 182189f6a..c3aa5b56d 100644 --- a/client/internal/engine_test.go +++ b/client/internal/engine_test.go @@ -1728,7 +1728,7 @@ func TestEngine_hasIPv6Changed(t *testing.T) { { name: "no v6 before, v6 added", current: v4Only, - confV6: netiputil.EncodeAddr(netip.MustParseAddr("fd00::1")), + confV6: netiputil.EncodePrefix(netip.MustParsePrefix("fd00::1/64")), expected: true, }, { @@ -1740,13 +1740,19 @@ func TestEngine_hasIPv6Changed(t *testing.T) { { name: "had v6, same v6", current: v4v6, - confV6: netiputil.EncodeAddr(netip.MustParseAddr("fd00::1")), + confV6: netiputil.EncodePrefix(netip.MustParsePrefix("fd00::1/64")), expected: false, }, { name: "had v6, different v6", current: v4v6, - confV6: netiputil.EncodeAddr(netip.MustParseAddr("fd00::2")), + confV6: netiputil.EncodePrefix(netip.MustParsePrefix("fd00::2/64")), + expected: true, + }, + { + name: "same v6 addr, different prefix length", + current: v4v6, + confV6: netiputil.EncodePrefix(netip.MustParsePrefix("fd00::1/80")), expected: true, }, { @@ -1760,9 +1766,7 @@ func TestEngine_hasIPv6Changed(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { engine := &Engine{ - wgInterface: &MockWGIface{ - AddressFunc: func() wgaddr.Address { return tt.current }, - }, + config: &EngineConfig{WgAddr: tt.current}, } conf := &mgmtProto.PeerConfig{ AddressV6: tt.confV6,