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 {