From 51067f162f01144be61328402b4874392b5ac658 Mon Sep 17 00:00:00 2001 From: Kuzz007 Date: Tue, 4 Aug 2026 10:28:56 +0300 Subject: [PATCH] Attach: allocate fresh when re-attaching with no active tunnel The previous fix (82cc69f5) made Attach's own address-reuse correctly not collide with itself across inbounds -- but it still always reused an identity's stored AllowedIPs verbatim, even when that identity currently has zero WireGuard/AmneziaWG attachments at all. A real report from testing this live: an identity fully detached from both its wg and awg inbounds, then re-attached, got its old address back even though several lower addresses were free -- because nothing about being fully detached ever cleared the stored value Attach copies from. Add hasTunnelAttachment, checked once against the identity's CURRENT inbound set before Attach's loop runs: if none of its current inbounds is WireGuard/AmneziaWG, clear the stored AllowedIPs so this attach allocates fresh (matching what a brand-new client would get) instead of resurrecting an address nothing reserves anymore. Left alone when the identity already has an active tunnel elsewhere, so extending it to a second protocol still keeps a consistent address. --- internal/web/service/client_attach_test.go | 56 ++++++++++++++++++++++ internal/web/service/client_crud.go | 29 +++++++++++ 2 files changed, 85 insertions(+) create mode 100644 internal/web/service/client_attach_test.go diff --git a/internal/web/service/client_attach_test.go b/internal/web/service/client_attach_test.go new file mode 100644 index 000000000..9058cd0fa --- /dev/null +++ b/internal/web/service/client_attach_test.go @@ -0,0 +1,56 @@ +package service + +import ( + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +// TestHasTunnelAttachmentDetectsWireguardOrAmneziaWG backs the fix for a +// real production bug: Attach copies an identity's stored AllowedIPs into +// every inbound it processes (so the same person keeps the same tunnel +// address across protocols), but when an identity has been fully detached +// from every WireGuard/AmneziaWG inbound, that stored address is a leftover +// nothing reserves anymore -- reusing it can skip past address space that's +// genuinely free (a real user's own case: address .21 resurrected instead +// of the actually-free .3). hasTunnelAttachment is what Attach checks to +// decide whether to clear the stored address before its loop, so it needs +// to correctly tell "still has an active tunnel elsewhere" (preserve) apart +// from "no tunnel attachment at all" (clear, allocate fresh). +func TestHasTunnelAttachmentDetectsWireguardOrAmneziaWG(t *testing.T) { + setupConflictDB(t) + seedInboundConflict(t, "awg-1", "0.0.0.0", 443, model.AmneziaWG, ``, `{"server":{"subnetIp":"10.8.1.0","subnetCidr":24},"clients":[]}`) + seedInboundConflict(t, "wg-1", "0.0.0.0", 51820, model.WireGuard, ``, `{"clients":[]}`) + seedInboundConflict(t, "vless-1", "0.0.0.0", 8443, model.VLESS, `{"network":"tcp"}`, `{"clients":[]}`) + + var awgInbound, wgInbound, vlessInbound model.Inbound + if err := database.GetDB().Where("tag = ?", "awg-1").First(&awgInbound).Error; err != nil { + t.Fatalf("read seeded awg row: %v", err) + } + if err := database.GetDB().Where("tag = ?", "wg-1").First(&wgInbound).Error; err != nil { + t.Fatalf("read seeded wg row: %v", err) + } + if err := database.GetDB().Where("tag = ?", "vless-1").First(&vlessInbound).Error; err != nil { + t.Fatalf("read seeded vless row: %v", err) + } + + s := &ClientService{} + inboundSvc := &InboundService{} + + if s.hasTunnelAttachment(inboundSvc, nil) { + t.Error("empty inboundIds must report no tunnel attachment") + } + if s.hasTunnelAttachment(inboundSvc, []int{vlessInbound.Id}) { + t.Error("a VLESS-only attachment must not count as a tunnel attachment") + } + if s.hasTunnelAttachment(inboundSvc, []int{99999}) { + t.Error("a nonexistent inbound id must not count as a tunnel attachment") + } + if !s.hasTunnelAttachment(inboundSvc, []int{vlessInbound.Id, wgInbound.Id}) { + t.Error("a WireGuard inbound among others must count as a tunnel attachment") + } + if !s.hasTunnelAttachment(inboundSvc, []int{awgInbound.Id}) { + t.Error("an AmneziaWG inbound must count as a tunnel attachment") + } +} diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index 9f9eaff0b..e675f43fd 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -603,6 +603,23 @@ func (s *ClientService) Delete(inboundSvc *InboundService, id int, keepTraffic b return needRestart, nil } +// hasTunnelAttachment reports whether any of inboundIds is a currently +// existing WireGuard or AmneziaWG inbound. Inbounds that fail to load are +// skipped rather than treated as an error -- Attach's own loop already +// surfaces a real error for any inbound it can't load when it gets there. +func (s *ClientService) hasTunnelAttachment(inboundSvc *InboundService, inboundIds []int) bool { + for _, ibId := range inboundIds { + inbound, err := inboundSvc.GetInbound(ibId) + if err != nil { + continue + } + if inbound.Protocol == model.WireGuard || inbound.Protocol == model.AmneziaWG { + return true + } + } + return false +} + func (s *ClientService) Attach(inboundSvc *InboundService, id int, inboundIds []int) (bool, error) { existing, err := s.GetByID(id) if err != nil { @@ -625,6 +642,18 @@ func (s *ClientService) Attach(inboundSvc *InboundService, id int, inboundIds [] clientWire.Flow = flow clientWire.UpdatedAt = time.Now().UnixMilli() + // If this identity has no CURRENT WireGuard/AmneziaWG attachment, + // clientWire.AllowedIPs (from the ClientRecord) is a leftover from + // whenever it last had one -- nothing reserves it anymore. Clear it so + // attaching to a tunnel inbound now allocates a fresh address instead + // of resurrecting the old one, which may no longer even be the lowest + // free slot. Left untouched when the identity already has an active + // tunnel elsewhere, so extending it to a second protocol still keeps + // the same address on both. + if !s.hasTunnelAttachment(inboundSvc, currentIds) { + clientWire.AllowedIPs = nil + } + emailSubIDs, sidErr := inboundSvc.getAllEmailSubIDs() if sidErr != nil { return false, sidErr