From 5a7b3b737002fbc81d21893b294840b9539d33d7 Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sun, 5 Jul 2026 21:17:25 +0200 Subject: [PATCH] fix(client): stop duplicate client entries accumulating in inbound settings Adding a user to multi-node inbounds could leave 3-6 identical entries in one inbound's settings.clients array: addInboundClient appended incoming clients unconditionally, and the duplicate-email precheck exempts a matching subId (so one identity can span several inbounds), so a retried or raced add of the same client re-appended it to an inbound that already carried it - on the master and, since nodes run the same code, on every node, whose snapshot adoption then copied the duplicates back verbatim. The normalized clients/client_inbounds tables stayed clean (unique constraints), which is why the phantom rows only showed in settings-driven views like the Detach clients modal, where duplicate React keys also broke the selection counter. Three layers: addInboundClient now skips incoming clients whose email is already on the target inbound (idempotent re-adds instead of duplication), node snapshot adoption collapses duplicate emails before writing the central row, and an idempotent startup repair rewrites any inbound whose settings still carry duplicates from older builds. Closes #5770 --- internal/database/db.go | 63 ++++++++++++ internal/database/inbound_dedupe_test.go | 78 +++++++++++++++ .../web/service/client_add_dedupe_test.go | 95 +++++++++++++++++++ internal/web/service/client_inbound_apply.go | 44 ++++++++- internal/web/service/client_locks.go | 43 +++++++++ internal/web/service/inbound_node.go | 3 + 6 files changed, 321 insertions(+), 5 deletions(-) create mode 100644 internal/database/inbound_dedupe_test.go create mode 100644 internal/web/service/client_add_dedupe_test.go diff --git a/internal/database/db.go b/internal/database/db.go index b67b75b4b..2e4ab9ea5 100644 --- a/internal/database/db.go +++ b/internal/database/db.go @@ -116,6 +116,9 @@ func initModels() error { if err := repairOverflowedTrafficCounters(); err != nil { return err } + if err := dedupeInboundSettingsClients(); err != nil { + return err + } if err := migrateLegacySocksInboundsToMixed(); err != nil { return err } @@ -566,6 +569,66 @@ func repairOverflowedTrafficCounters() error { return nil } +// dedupeInboundSettingsClients collapses duplicate same-email entries inside +// every inbound's settings.clients array, keeping the first occurrence. +// Retried or raced multi-node client adds on older builds appended the same +// client several times (#5770), which the client lists then rendered as +// phantom duplicates. Runs on every start (idempotent, writes only changed +// rows) because a restored backup or a not-yet-upgraded node's snapshot can +// reintroduce duplicates. +func dedupeInboundSettingsClients() error { + var inbounds []model.Inbound + if err := db.Find(&inbounds).Error; err != nil { + return err + } + repaired := int64(0) + for _, inbound := range inbounds { + if strings.TrimSpace(inbound.Settings) == "" { + continue + } + var settings map[string]any + if err := json.Unmarshal([]byte(inbound.Settings), &settings); err != nil { + continue + } + clients, _ := settings["clients"].([]any) + if len(clients) < 2 { + continue + } + seen := make(map[string]struct{}, len(clients)) + kept := make([]any, 0, len(clients)) + for _, c := range clients { + if cm, ok := c.(map[string]any); ok { + if email, _ := cm["email"].(string); email != "" { + key := strings.ToLower(email) + if _, dup := seen[key]; dup { + continue + } + seen[key] = struct{}{} + } + } + kept = append(kept, c) + } + if len(kept) == len(clients) { + continue + } + settings["clients"] = kept + newSettings, err := json.MarshalIndent(settings, "", " ") + if err != nil { + log.Printf("dedupeInboundSettingsClients: skip inbound %d (marshal failed): %v", inbound.Id, err) + continue + } + if err := db.Model(&model.Inbound{}).Where("id = ?", inbound.Id). + Update("settings", string(newSettings)).Error; err != nil { + return err + } + repaired++ + } + if repaired > 0 { + log.Printf("Removed duplicate client entries from %d inbound(s)", repaired) + } + return nil +} + func isIgnorableDuplicateColumnErr(err error, mdl any) bool { if err == nil { return false diff --git a/internal/database/inbound_dedupe_test.go b/internal/database/inbound_dedupe_test.go new file mode 100644 index 000000000..8a7cee966 --- /dev/null +++ b/internal/database/inbound_dedupe_test.go @@ -0,0 +1,78 @@ +package database + +import ( + "encoding/json" + "path/filepath" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +// TestDedupeInboundSettingsClients_CollapsesDuplicateEmails covers the #5770 +// repair: settings.clients arrays written by older builds can carry the same +// email several times; startup must collapse them to the first occurrence and +// leave clean inbounds byte-for-byte untouched. +func TestDedupeInboundSettingsClients_CollapsesDuplicateEmails(t *testing.T) { + dbDir := t.TempDir() + t.Setenv("XUI_DB_FOLDER", dbDir) + if err := InitDB(filepath.Join(dbDir, "x-ui.db")); err != nil { + t.Fatalf("InitDB failed: %v", err) + } + t.Cleanup(func() { _ = CloseDB() }) + + dupSettings := `{"clients": [` + + `{"id": "u1", "email": "dup@x", "subId": "s1", "enable": true},` + + `{"id": "u2", "email": "keep@x", "subId": "s2", "enable": true},` + + `{"id": "u1", "email": "dup@x", "subId": "s1", "enable": true},` + + `{"id": "u1", "email": "dup@x", "subId": "s1", "enable": true}]}` + dirty := model.Inbound{UserId: 1, Port: 21001, Protocol: model.VLESS, Tag: "dedupe-dirty", Settings: dupSettings} + if err := db.Create(&dirty).Error; err != nil { + t.Fatalf("create dirty inbound: %v", err) + } + + cleanSettings := `{"clients": [{"id": "u3", "email": "solo@x", "subId": "s3", "enable": true}]}` + clean := model.Inbound{UserId: 1, Port: 21002, Protocol: model.VLESS, Tag: "dedupe-clean", Settings: cleanSettings} + if err := db.Create(&clean).Error; err != nil { + t.Fatalf("create clean inbound: %v", err) + } + + if err := dedupeInboundSettingsClients(); err != nil { + t.Fatalf("dedupeInboundSettingsClients: %v", err) + } + + var gotDirty model.Inbound + if err := db.First(&gotDirty, dirty.Id).Error; err != nil { + t.Fatalf("reload dirty inbound: %v", err) + } + var parsed struct { + Clients []map[string]any `json:"clients"` + } + if err := json.Unmarshal([]byte(gotDirty.Settings), &parsed); err != nil { + t.Fatalf("parse repaired settings: %v", err) + } + if len(parsed.Clients) != 2 { + t.Fatalf("expected 2 clients after dedupe, got %d: %s", len(parsed.Clients), gotDirty.Settings) + } + if parsed.Clients[0]["email"] != "dup@x" || parsed.Clients[1]["email"] != "keep@x" { + t.Fatalf("expected first occurrences [dup@x keep@x], got %v", parsed.Clients) + } + + var gotClean model.Inbound + if err := db.First(&gotClean, clean.Id).Error; err != nil { + t.Fatalf("reload clean inbound: %v", err) + } + if gotClean.Settings != cleanSettings { + t.Fatalf("clean inbound settings were rewritten:\nbefore: %s\nafter: %s", cleanSettings, gotClean.Settings) + } + + if err := dedupeInboundSettingsClients(); err != nil { + t.Fatalf("second dedupe run: %v", err) + } + var again model.Inbound + if err := db.First(&again, dirty.Id).Error; err != nil { + t.Fatalf("reload after second run: %v", err) + } + if again.Settings != gotDirty.Settings { + t.Fatal("dedupe is not idempotent: settings changed on the second run") + } +} diff --git a/internal/web/service/client_add_dedupe_test.go b/internal/web/service/client_add_dedupe_test.go new file mode 100644 index 000000000..ae3c95f5c --- /dev/null +++ b/internal/web/service/client_add_dedupe_test.go @@ -0,0 +1,95 @@ +package service + +import ( + "encoding/json" + "testing" + + "github.com/google/uuid" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +func mustUnmarshal(t *testing.T, raw string, v any) { + t.Helper() + if err := json.Unmarshal([]byte(raw), v); err != nil { + t.Fatalf("unmarshal %q: %v", raw, err) + } +} + +func settingsClientEmails(t *testing.T, inboundId int) []string { + t.Helper() + var ib model.Inbound + if err := database.GetDB().First(&ib, inboundId).Error; err != nil { + t.Fatalf("load inbound %d: %v", inboundId, err) + } + clients, err := (&InboundService{}).GetClients(&ib) + if err != nil { + t.Fatalf("GetClients: %v", err) + } + emails := make([]string, 0, len(clients)) + for _, c := range clients { + emails = append(emails, c.Email) + } + return emails +} + +// Re-adding a client that is already on the inbound must be an idempotent +// no-op, not a second settings entry: checkEmailsExistForClients exempts a +// matching subId (so one identity can span inbounds), which let retried or +// raced adds duplicate the same email inside one settings array (#5770). +func TestAddInboundClient_SkipsClientsAlreadyOnInbound(t *testing.T) { + setupBulkDB(t) + nodeID, _ := setupNodeRuntime(t) + + alice := model.Client{ID: uuid.NewString(), Email: "alice@dup", SubID: "alice-sub-1234567", Enable: true} + ib := nodeInbound(t, nodeID, 33001, []model.Client{alice}) + + svc := &ClientService{} + inboundSvc := &InboundService{} + + if _, err := svc.AddInboundClient(inboundSvc, &model.Inbound{Id: ib.Id, Protocol: model.VLESS, Settings: clientsSettings(t, []model.Client{alice})}); err != nil { + t.Fatalf("re-add of existing client should be a no-op, got error: %v", err) + } + if emails := settingsClientEmails(t, ib.Id); len(emails) != 1 || emails[0] != "alice@dup" { + t.Fatalf("settings after duplicate re-add: expected exactly [alice@dup], got %v", emails) + } + + bob := model.Client{ID: uuid.NewString(), Email: "bob@dup", SubID: "bob-sub-123456789", Enable: true} + if _, err := svc.AddInboundClient(inboundSvc, &model.Inbound{Id: ib.Id, Protocol: model.VLESS, Settings: clientsSettings(t, []model.Client{alice, bob})}); err != nil { + t.Fatalf("mixed add (one duplicate, one new): %v", err) + } + if emails := settingsClientEmails(t, ib.Id); len(emails) != 2 || emails[0] != "alice@dup" || emails[1] != "bob@dup" { + t.Fatalf("settings after mixed add: expected [alice@dup bob@dup], got %v", emails) + } +} + +func TestDedupeSettingsClients(t *testing.T) { + dup := `{"clients": [` + + `{"id": "u1", "email": "a@x", "subId": "s1"},` + + `{"id": "u2", "email": "b@x", "subId": "s2"},` + + `{"id": "u1", "email": "a@x", "subId": "s1"},` + + `{"id": "u1", "email": "A@X", "subId": "s1"}]}` + out, changed := dedupeSettingsClients(dup) + if !changed { + t.Fatal("expected duplicates to be removed") + } + var parsed struct { + Clients []model.Client `json:"clients"` + } + mustUnmarshal(t, out, &parsed) + if len(parsed.Clients) != 2 || parsed.Clients[0].Email != "a@x" || parsed.Clients[1].Email != "b@x" { + t.Fatalf("expected first occurrences [a@x b@x], got %+v", parsed.Clients) + } + + clean := `{"clients": [{"id": "u1", "email": "a@x"}, {"id": "u2", "email": "b@x"}]}` + if _, changed := dedupeSettingsClients(clean); changed { + t.Fatal("clean settings must not be rewritten") + } + if _, changed := dedupeSettingsClients(""); changed { + t.Fatal("empty settings must not be rewritten") + } + if _, changed := dedupeSettingsClients("{not json"); changed { + t.Fatal("invalid settings must not be rewritten") + } +} diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index ad38760ae..14f8fd10c 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -319,12 +319,46 @@ func (s *ClientService) addInboundClient(inboundSvc *InboundService, data *model return false, err } - if oldInbound.Protocol == model.WireGuard { - existing, gcErr := inboundSvc.GetClients(oldInbound) - if gcErr != nil { - return false, gcErr + existingClients, err := inboundSvc.GetClients(oldInbound) + if err != nil { + return false, err + } + + // A client already on this inbound is skipped instead of appended again: + // checkEmailsExistForClients exempts a matching subId so one identity can + // live on several inbounds, which let retried or raced adds duplicate the + // same email inside a single settings array (#5770). clients and + // interfaceClients are parsed from the same data.Settings array, so they + // stay index-aligned while filtering. + if len(existingClients) > 0 && len(clients) > 0 { + existingEmails := make(map[string]struct{}, len(existingClients)) + for _, c := range existingClients { + if c.Email != "" { + existingEmails[strings.ToLower(c.Email)] = struct{}{} + } } - if dErr := defaultWireguardClients(existing, clients, interfaceClients); dErr != nil { + keptClients := make([]model.Client, 0, len(clients)) + keptWire := make([]any, 0, len(interfaceClients)) + for i, c := range clients { + if c.Email != "" { + if _, dup := existingEmails[strings.ToLower(c.Email)]; dup { + continue + } + } + keptClients = append(keptClients, c) + if i < len(interfaceClients) { + keptWire = append(keptWire, interfaceClients[i]) + } + } + if len(keptClients) == 0 { + return false, nil + } + clients = keptClients + interfaceClients = keptWire + } + + if oldInbound.Protocol == model.WireGuard { + if dErr := defaultWireguardClients(existingClients, clients, interfaceClients); dErr != nil { return false, dErr } } diff --git a/internal/web/service/client_locks.go b/internal/web/service/client_locks.go index 9a299688f..3ea0f9beb 100644 --- a/internal/web/service/client_locks.go +++ b/internal/web/service/client_locks.go @@ -2,6 +2,7 @@ package service import ( "encoding/json" + "strings" "sync" "time" @@ -141,6 +142,48 @@ func isClientEmailTombstoned(email string) bool { return true } +// dedupeSettingsClients collapses duplicate same-email client entries inside a +// settings JSON blob, keeping the first occurrence. Node snapshots produced by +// builds without the addInboundClient duplicate guard can carry duplicates +// (#5770); adopting them verbatim would copy the duplication into the central +// inbound. Returns the filtered JSON and whether anything was removed. +func dedupeSettingsClients(settings string) (string, bool) { + if settings == "" { + return settings, false + } + var parsed map[string]any + if err := json.Unmarshal([]byte(settings), &parsed); err != nil { + return settings, false + } + clients, _ := parsed["clients"].([]any) + if len(clients) < 2 { + return settings, false + } + seen := make(map[string]struct{}, len(clients)) + kept := make([]any, 0, len(clients)) + for _, c := range clients { + if cm, ok := c.(map[string]any); ok { + if email, _ := cm["email"].(string); email != "" { + key := strings.ToLower(email) + if _, dup := seen[key]; dup { + continue + } + seen[key] = struct{}{} + } + } + kept = append(kept, c) + } + if len(kept) == len(clients) { + return settings, false + } + parsed["clients"] = kept + b, err := json.MarshalIndent(parsed, "", " ") + if err != nil { + return settings, false + } + return string(b), true +} + // stripTombstonedClients drops just-deleted client entries from a node // snapshot's settings JSON so adopting a stale snapshot can't re-add them to // the central inbound while the delete tombstone is live. Returns the filtered diff --git a/internal/web/service/inbound_node.go b/internal/web/service/inbound_node.go index 80662947a..0c49ba2c8 100644 --- a/internal/web/service/inbound_node.go +++ b/internal/web/service/inbound_node.go @@ -534,6 +534,9 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi if stripped, changed := stripTombstonedClients(adoptedSettings); changed { adoptedSettings = stripped } + if deduped, changed := dedupeSettingsClients(adoptedSettings); changed { + adoptedSettings = deduped + } updates := map[string]any{} if !dirty {