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.
This commit is contained in:
Kuzz007 2026-08-04 10:28:56 +03:00
parent 3c974e4a71
commit c1c87fd351
2 changed files with 85 additions and 0 deletions

View file

@ -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")
}
}

View file

@ -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