From a31fa9abfa4d316ff77608da0326c4ade08acd84 Mon Sep 17 00:00:00 2001 From: Sanaei Date: Thu, 3 Sep 2026 18:06:35 +0200 Subject: [PATCH] fix(node): refuse a node's claim on another inbound's client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sync adopts each node's reported clients through SyncInbound, which resolves a client record by email alone — and clients.email is globally unique. A node reporting a colliding email therefore overwrote that client's UUID even when the client is attached only to a master inbound, and the master then rebuilt its own Xray config with the node-supplied credential: the real user locked out. Skip a reported client whose record is attached only to inbounds of other nodes. A record attached nowhere stays adoptable, so the soft-orphan reattach path a flapping node depends on is unaffected. --- internal/web/service/inbound_node.go | 65 ++++++++++++++ .../web/service/node_foreign_client_test.go | 90 +++++++++++++++++++ 2 files changed, 155 insertions(+) create mode 100644 internal/web/service/node_foreign_client_test.go diff --git a/internal/web/service/inbound_node.go b/internal/web/service/inbound_node.go index 0fc458b27..775fa6773 100644 --- a/internal/web/service/inbound_node.go +++ b/internal/web/service/inbound_node.go @@ -21,6 +21,8 @@ import ( var reportedRemoteTagConflict sync.Map +var reportedForeignClientClaim sync.Map + // nodeBulkPushThreshold caps how many per-client RPCs a single operation will // stream to a remote node. Above it, the panel marks the node dirty instead and // lets one ReconcileNode push converge the whole inbound — far cheaper than M @@ -412,6 +414,46 @@ func adoptedWireInbound(c, snapIb *model.Inbound, adoptedSettings string) *model return &a } +// clientEmailsOwnedElsewhere returns the emails attached only to inbounds of +// other nodes: email is unique, so adopting one would overwrite a client this +// node does not serve. Attached nowhere means soft-orphaned, hence adoptable. +func clientEmailsOwnedElsewhere(tx *gorm.DB, nodeID int, emails []string) (map[string]struct{}, error) { + attachedEmails := func(nodeScoped bool) ([]string, error) { + q := tx.Table("clients"). + Joins("JOIN client_inbounds ON client_inbounds.client_id = clients.id"). + Joins("JOIN inbounds ON inbounds.id = client_inbounds.inbound_id"). + Where("clients.email IN ?", emails) + if nodeScoped { + q = q.Where("inbounds.node_id = ?", nodeID) + } + var rows []string + err := q.Pluck("clients.email", &rows).Error + return rows, err + } + attached, err := attachedEmails(false) + if err != nil { + return nil, err + } + if len(attached) == 0 { + return nil, nil + } + owned, err := attachedEmails(true) + if err != nil { + return nil, err + } + ownedSet := make(map[string]struct{}, len(owned)) + for _, email := range owned { + ownedSet[email] = struct{}{} + } + foreign := make(map[string]struct{}) + for _, email := range attached { + if _, ok := ownedSet[email]; !ok { + foreign[email] = struct{}{} + } + } + return foreign, nil +} + func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.TrafficSnapshot, dirty, justPushed bool) (bool, error) { if snap == nil || nodeID <= 0 { return false, nil @@ -1184,6 +1226,29 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi } } } + if len(localEmails) > 0 { + foreign, err := clientEmailsOwnedElsewhere(tx, nodeID, localEmails) + if err != nil { + return false, err + } + if len(foreign) > 0 { + kept := filtered[:0] + for i := range filtered { + if _, claimed := foreign[filtered[i].Email]; !claimed { + kept = append(kept, filtered[i]) + continue + } + key := fmt.Sprintf("%d:%s", nodeID, filtered[i].Email) + if _, seen := reportedForeignClientClaim.LoadOrStore(key, struct{}{}); !seen { + logger.Warningf( + "setRemoteTraffic: node %d reported client %q, which is attached only to inbounds of another node — not adopting (rename one side to remove the duplicate email)", + nodeID, filtered[i].Email, + ) + } + } + filtered = kept + } + } if err := s.clientService.SyncInbound(tx, c.Id, filtered); err != nil { logger.Warningf("setRemoteTraffic: sync clients for tag %q failed: %v", snapIb.Tag, err) syncFailedInbounds[c.Id] = struct{}{} diff --git a/internal/web/service/node_foreign_client_test.go b/internal/web/service/node_foreign_client_test.go new file mode 100644 index 000000000..ee7c09bee --- /dev/null +++ b/internal/web/service/node_foreign_client_test.go @@ -0,0 +1,90 @@ +package service + +import ( + "fmt" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database/model" + "github.com/mhsanaei/3x-ui/v3/internal/xray" + + "gorm.io/gorm" +) + +func readClientUUID(t *testing.T, db *gorm.DB, email string) string { + t.Helper() + var row model.ClientRecord + if err := db.Where("email = ?", email).First(&row).Error; err != nil { + t.Fatalf("read client %q: %v", email, err) + } + return row.UUID +} + +// Emails are globally unique, so a node reporting one that belongs to a master +// inbound would otherwise overwrite its credentials and lock the real user out. +func TestNodeCannotClaimClientOfAnotherInbound(t *testing.T) { + db := initTrafficTestDB(t) + svc := &InboundService{} + clientSvc := &ClientService{} + + seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true}) + + const ( + victim = "victim@x" + nodeLocal = "nodelocal@x" + legitUUID = "11111111-1111-1111-1111-111111111111" + attackUUID = "99999999-9999-9999-9999-999999999999" + ) + + master := &model.Inbound{ + UserId: 1, Tag: "master-in", Enable: true, Port: 40001, Protocol: model.VLESS, + Settings: fmt.Sprintf(`{"clients":[{"email":%q,"id":%q,"enable":true}]}`, victim, legitUUID), + } + if err := db.Create(master).Error; err != nil { + t.Fatalf("create master inbound: %v", err) + } + masterClients, err := svc.GetClients(master) + if err != nil { + t.Fatalf("parse master clients: %v", err) + } + if err := clientSvc.SyncInbound(db, master.Id, masterClients); err != nil { + t.Fatalf("attach master client: %v", err) + } + if got := readClientUUID(t, db, victim); got != legitUUID { + t.Fatalf("setup: master client uuid = %q, want %q", got, legitUUID) + } + + createNodeInbound(t, db, 1, "n1-in", 41001) + hostile := fmt.Sprintf(`{"clients":[{"email":%q,"id":%q,"enable":true},{"email":%q,"id":%q,"enable":true}]}`, + victim, attackUUID, nodeLocal, attackUUID) + syncNodeWithSettings(t, svc, 1, "n1-in", hostile, + xray.ClientTraffic{Email: victim, Enable: true}, + xray.ClientTraffic{Email: nodeLocal, Enable: true}) + + if got := readClientUUID(t, db, victim); got != legitUUID { + t.Fatalf("node overwrote a master client's uuid: got %q, want %q", got, legitUUID) + } + nodeAttached, err := clientSvc.ListForInbound(db, nodeInboundID(t, db, "n1-in")) + if err != nil { + t.Fatalf("list node clients: %v", err) + } + for _, c := range nodeAttached { + if c.Email == victim { + t.Fatal("node inbound adopted a client that belongs to a master inbound") + } + } + + // The node's own client must still be adopted, or the guard has replaced one + // bug with a worse one. + if got := readClientUUID(t, db, nodeLocal); got != attackUUID { + t.Fatalf("node-owned client not adopted: uuid = %q, want %q", got, attackUUID) + } +} + +func nodeInboundID(t *testing.T, db *gorm.DB, tag string) int { + t.Helper() + var ib model.Inbound + if err := db.Where("tag = ?", tag).First(&ib).Error; err != nil { + t.Fatalf("read inbound %q: %v", tag, err) + } + return ib.Id +}