From 7214ffafc595e545f3cc419bb753ae61c19ca5e8 Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Mon, 11 May 2026 12:51:45 +0200 Subject: [PATCH] fix(inbounds): scope port check to node and preserve caller tag Different nodes are different machines, so same port + transport across NodeIDs shouldn't conflict. resolveInboundTag now keeps a caller-supplied unique tag verbatim so central and node panels stay in agreement instead of regenerating into a UNIQUE constraint failure on sync. --- web/service/inbound.go | 11 +-- web/service/port_conflict.go | 53 ++++++++++++- web/service/port_conflict_test.go | 125 ++++++++++++++++++++++++++++++ 3 files changed, 176 insertions(+), 13 deletions(-) diff --git a/web/service/inbound.go b/web/service/inbound.go index 65c60826..b0d42533 100644 --- a/web/service/inbound.go +++ b/web/service/inbound.go @@ -291,12 +291,7 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo return inbound, false, common.NewError("Port already exists:", inbound.Port) } - // pick a tag that won't collide with an existing row. for the common - // case this is the same "inbound-" string the controller already - // set; only when this port already has another inbound on a different - // transport (now possible after the transport-aware port check) does - // this disambiguate with a -tcp/-udp suffix. see #4103. - inbound.Tag, err = s.generateInboundTag(inbound, 0) + inbound.Tag, err = s.resolveInboundTag(inbound, 0) if err != nil { return inbound, false, err } @@ -636,9 +631,7 @@ func (s *InboundService) UpdateInbound(inbound *model.Inbound) (*model.Inbound, oldInbound.Settings = inbound.Settings oldInbound.StreamSettings = inbound.StreamSettings oldInbound.Sniffing = inbound.Sniffing - // regenerate tag with collision-aware logic. for this row we pass - // inbound.Id as ignoreId so it doesn't see its own old tag in the db. - oldInbound.Tag, err = s.generateInboundTag(inbound, inbound.Id) + oldInbound.Tag, err = s.resolveInboundTag(inbound, inbound.Id) if err != nil { return inbound, false, err } diff --git a/web/service/port_conflict.go b/web/service/port_conflict.go index 0eb01464..a2dd2183 100644 --- a/web/service/port_conflict.go +++ b/web/service/port_conflict.go @@ -118,15 +118,16 @@ func isAnyListen(s string) bool { // port-only check, this one understands that tcp/443 and udp/443 are // independent sockets in linux and may coexist on the same address. // +// node scope: inbounds with different NodeID run on different physical +// machines (local panel xray vs a remote node, or two remote nodes), +// so their sockets can't collide. only candidates with the same NodeID +// participate in the listen/transport overlap check. +// // the listen-overlap rule (specific addr conflicts with any-addr on the // same port, both directions) is preserved from the previous check. func (s *InboundService) checkPortConflict(inbound *model.Inbound, ignoreId int) (bool, error) { db := database.GetDB() - // pull every candidate on this port; we filter by listen-overlap and - // transport in go to keep the sql plain. the port column is indexed - // in practice by the existing port check, and the candidate set is - // tiny (one per coexisting socket family at most). var candidates []*model.Inbound q := db.Model(model.Inbound{}).Where("port = ?", inbound.Port) if ignoreId > 0 { @@ -138,6 +139,9 @@ func (s *InboundService) checkPortConflict(inbound *model.Inbound, ignoreId int) newBits := inboundTransports(inbound.Protocol, inbound.StreamSettings, inbound.Settings) for _, c := range candidates { + if !sameNode(c.NodeID, inbound.NodeID) { + continue + } if !listenOverlaps(c.Listen, inbound.Listen) { continue } @@ -148,6 +152,21 @@ func (s *InboundService) checkPortConflict(inbound *model.Inbound, ignoreId int) return false, nil } +// sameNode reports whether two NodeID pointers refer to the same xray +// process. nil/nil means both inbounds run on the local panel; non-nil +// with equal value means they share the same remote node. any mix +// (local vs remote, remote-A vs remote-B) is "different node" and +// can't produce a real socket collision. +func sameNode(a, b *int) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return *a == *b +} + // baseInboundTag is the historical "inbound-" / "inbound-:" // shape. kept exactly so existing routing rules that reference these tags // keep working after the upgrade. @@ -220,6 +239,32 @@ func (s *InboundService) generateInboundTag(inbound *model.Inbound, ignoreId int return "", common.NewError("could not pick a unique inbound tag for port:", inbound.Port) } +// resolveInboundTag chooses a tag for an Add or Update. when the caller +// supplied a non-empty Tag (e.g. the central panel pushed its picked +// tag to a node during a multi-node sync) and that tag is free in the +// local DB, it's used verbatim so the two panels stay in agreement — +// otherwise the node would regenerate (often back to bare +// "inbound-") and the eventual traffic sync-back would try to +// INSERT a row whose tag already exists, hitting the UNIQUE constraint +// on inbounds.tag and rolling the node-side row right back out. +// when Tag is empty (the common UI path) or collides, fall back to the +// transport-aware generateInboundTag. +// +// ignoreId mirrors generateInboundTag: pass 0 on add, the inbound's +// own id on update so a row doesn't see its own current tag as taken. +func (s *InboundService) resolveInboundTag(inbound *model.Inbound, ignoreId int) (string, error) { + if inbound.Tag != "" { + taken, err := s.tagExists(inbound.Tag, ignoreId) + if err != nil { + return "", err + } + if !taken { + return inbound.Tag, nil + } + } + return s.generateInboundTag(inbound, ignoreId) +} + func (s *InboundService) tagExists(tag string, ignoreId int) (bool, error) { db := database.GetDB() q := db.Model(model.Inbound{}).Where("tag = ?", tag) diff --git a/web/service/port_conflict_test.go b/web/service/port_conflict_test.go index 984ea52f..1a7f0c1e 100644 --- a/web/service/port_conflict_test.go +++ b/web/service/port_conflict_test.go @@ -35,6 +35,11 @@ func setupConflictDB(t *testing.T) { } func seedInboundConflict(t *testing.T, tag, listen string, port int, protocol model.Protocol, streamSettings, settings string) { + t.Helper() + seedInboundConflictNode(t, tag, listen, port, protocol, streamSettings, settings, nil) +} + +func seedInboundConflictNode(t *testing.T, tag, listen string, port int, protocol model.Protocol, streamSettings, settings string, nodeID *int) { t.Helper() in := &model.Inbound{ Tag: tag, @@ -44,12 +49,15 @@ func seedInboundConflict(t *testing.T, tag, listen string, port int, protocol mo Protocol: protocol, StreamSettings: streamSettings, Settings: settings, + NodeID: nodeID, } if err := database.GetDB().Create(in).Error; err != nil { t.Fatalf("seed inbound %s: %v", tag, err) } } +func intPtr(v int) *int { return &v } + func TestInboundTransports(t *testing.T) { cases := []struct { name string @@ -345,6 +353,123 @@ func TestGenerateInboundTag_SpecificListenSameDisambiguation(t *testing.T) { } } +// inbounds bound to different nodes run on different physical machines, +// so the same port + transport must be allowed across nodes. covers +// local-vs-remote, remote-A-vs-remote-B, and the still-clashing +// same-node case. +func TestCheckPortConflict_NodeScope(t *testing.T) { + setupConflictDB(t) + seedInboundConflictNode(t, "local-443-tcp", "0.0.0.0", 443, model.VLESS, `{"network":"tcp"}`, `{}`, nil) + seedInboundConflictNode(t, "node1-443-tcp", "0.0.0.0", 443, model.VLESS, `{"network":"tcp"}`, `{}`, intPtr(1)) + + svc := &InboundService{} + + cases := []struct { + name string + nodeID *int + want bool + }{ + {"new local same port + tcp clashes with local", nil, true}, + {"new remote on different node from local is fine", intPtr(2), false}, + {"new remote on existing node 1 clashes", intPtr(1), true}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + candidate := &model.Inbound{ + Listen: "0.0.0.0", + Port: 443, + Protocol: model.VLESS, + StreamSettings: `{"network":"tcp"}`, + NodeID: c.nodeID, + } + got, err := svc.checkPortConflict(candidate, 0) + if err != nil { + t.Fatalf("checkPortConflict: %v", err) + } + if got != c.want { + t.Fatalf("got conflict=%v, want %v", got, c.want) + } + }) + } +} + +// when the caller passes an explicit non-empty Tag that doesn't collide, +// resolveInboundTag returns it verbatim. this is the cross-panel path: +// the central panel picks a tag, pushes the inbound to a node, and the +// node must keep that exact tag so the eventual traffic sync-back can +// match the row by tag. previously the node regenerated and the two +// panels diverged, causing a UNIQUE constraint failure on sync. +func TestResolveInboundTag_RespectsCallerTagWhenFree(t *testing.T) { + setupConflictDB(t) + seedInboundConflictNode(t, "inbound-5000", "0.0.0.0", 5000, model.VLESS, `{"network":"tcp"}`, `{}`, nil) + seedInboundConflictNode(t, "inbound-5000-udp", "0.0.0.0", 5000, model.Hysteria2, ``, ``, nil) + + svc := &InboundService{} + pushed := &model.Inbound{ + Tag: "inbound-5000-tcp", + Listen: "0.0.0.0", + Port: 5000, + Protocol: model.VLESS, + StreamSettings: `{"network":"tcp"}`, + NodeID: intPtr(1), + } + got, err := svc.resolveInboundTag(pushed, 0) + if err != nil { + t.Fatalf("resolveInboundTag: %v", err) + } + if got != "inbound-5000-tcp" { + t.Fatalf("caller tag must be preserved when free, got %q", got) + } +} + +// when the caller leaves Tag empty (the local UI path) resolveInboundTag +// falls back to generateInboundTag, which keeps the historical +// "inbound-" shape so existing routing rules don't change. +func TestResolveInboundTag_GeneratesWhenTagEmpty(t *testing.T) { + setupConflictDB(t) + + svc := &InboundService{} + in := &model.Inbound{ + Listen: "0.0.0.0", + Port: 8443, + Protocol: model.VLESS, + } + got, err := svc.resolveInboundTag(in, 0) + if err != nil { + t.Fatalf("resolveInboundTag: %v", err) + } + if got != "inbound-8443" { + t.Fatalf("expected generated inbound-8443, got %q", got) + } +} + +// when the caller's Tag collides (e.g. a node that was used standalone +// happens to already own the tag the central panel picked), +// resolveInboundTag falls back to generateInboundTag rather than +// failing — the inbound still lands, just under a slightly different +// tag that the central will pick up via the AddInbound response. +func TestResolveInboundTag_RegeneratesOnCollision(t *testing.T) { + setupConflictDB(t) + seedInboundConflictNode(t, "inbound-5000-tcp", "0.0.0.0", 5000, model.VLESS, `{"network":"tcp"}`, `{}`, nil) + + svc := &InboundService{} + pushed := &model.Inbound{ + Tag: "inbound-5000-tcp", + Listen: "0.0.0.0", + Port: 5000, + Protocol: model.Hysteria2, + StreamSettings: ``, + Settings: ``, + } + got, err := svc.resolveInboundTag(pushed, 0) + if err != nil { + t.Fatalf("resolveInboundTag: %v", err) + } + if got == "inbound-5000-tcp" { + t.Fatalf("colliding caller tag must be replaced, but resolver kept %q", got) + } +} + // updating an inbound must not see itself as a conflict, that's what // ignoreId is for. func TestCheckPortConflict_IgnoreSelfOnUpdate(t *testing.T) {