Browse Source

fix(ldap): stop auto-delete from wiping every client on an empty directory

FetchVlessFlags returns (empty map, nil) whenever the bind succeeds but the
search yields nothing usable — a renamed OU, a service account that lost read
on the user attribute, a filter that stopped matching. The only guard on the
destructive half of the sync was `err != nil`, so that answer was read as
"every user is gone" and the job detached every client from the configured
inbounds, once a minute, for as long as the directory stayed broken.

Gate auto-delete behind autoDeleteSafeForFetch: refuse an empty fetch, and
refuse one that collapsed below half of the last successful sync, which is a
misconfigured directory far more often than real churn.

Also stop splitCsv from defaulting an empty string to DefaultTruthyValues.
That default belongs to the truthy-value setting, but splitCsv is also what
parses ldapInboundTags, so an unconfigured tag list silently resolved to
["true","1","yes","on"]. It only ever bounded the blast radius by accident.
Sanaei 10 hours ago
parent
commit
f4b7b08e08
2 changed files with 67 additions and 3 deletions
  1. 34 0
      internal/web/job/ldap_auto_delete_guard_test.go
  2. 33 3
      internal/web/job/ldap_sync_job.go

+ 34 - 0
internal/web/job/ldap_auto_delete_guard_test.go

@@ -0,0 +1,34 @@
+package job
+
+import "testing"
+
+// A bind that succeeds with an unusable search returns (empty, nil); the only
+// guard was `err != nil`, so that answer detached the entire inbound.
+func TestAutoDeleteAllowed(t *testing.T) {
+	cases := []struct {
+		name     string
+		previous int64
+		fetched  int
+		want     bool
+	}{
+		{"empty fetch on a fresh job is refused", 0, 0, false},
+		{"empty fetch after a healthy sync is refused", 500, 0, false},
+		{"full fetch on a fresh job is allowed", 0, 500, true},
+		{"steady fetch is allowed", 500, 500, true},
+		{"growth is allowed", 500, 900, true},
+		{"shrink above the retention floor is allowed", 500, 250, true},
+		{"shrink below the retention floor is refused", 500, 249, false},
+		{"collapse to a single user is refused", 500, 1, false},
+		{"single user with no history is allowed", 0, 1, true},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			j := NewLdapSyncJob()
+			j.lastFlagCount.Store(c.previous)
+			if got := j.autoDeleteSafeForFetch(c.fetched); got != c.want {
+				t.Fatalf("autoDeleteSafeForFetch(previous=%d, fetched=%d) = %v, want %v",
+					c.previous, c.fetched, got, c.want)
+			}
+		})
+	}
+}

+ 33 - 3
internal/web/job/ldap_sync_job.go

@@ -2,6 +2,7 @@ package job
 
 import (
 	"strings"
+	"sync/atomic"
 	"time"
 
 	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
@@ -12,11 +13,16 @@ import (
 
 var DefaultTruthyValues = []string{"true", "1", "yes", "on"}
 
+// Share of the previous successful fetch a new one must still return to be
+// trusted: a sudden collapse means a broken directory far more often than churn.
+const ldapAutoDeleteMinRetainPercent = 50
+
 type LdapSyncJob struct {
 	settingService service.SettingService
 	inboundService service.InboundService
 	clientService  service.ClientService
 	xrayService    service.XrayService
+	lastFlagCount  atomic.Int64
 }
 
 // --- Helper functions for mustGet ---
@@ -77,7 +83,7 @@ func (j *LdapSyncJob) Run() {
 		UserFilter:         mustGetString(j.settingService.GetLdapUserFilter),
 		UserAttr:           mustGetString(j.settingService.GetLdapUserAttr),
 		FlagField:          mustGetStringOr(j.settingService.GetLdapFlagField, mustGetString(j.settingService.GetLdapVlessField)),
-		TruthyVals:         splitCsv(mustGetString(j.settingService.GetLdapTruthyValues)),
+		TruthyVals:         truthyValuesOrDefault(mustGetString(j.settingService.GetLdapTruthyValues)),
 		Invert:             mustGetBool(j.settingService.GetLdapInvertFlag),
 	}
 
@@ -157,7 +163,7 @@ func (j *LdapSyncJob) Run() {
 
 	// --- Auto delete clients not in LDAP ---
 	autoDelete := mustGetBool(j.settingService.GetLdapAutoDelete)
-	if autoDelete {
+	if autoDelete && j.autoDeleteSafeForFetch(len(flags)) {
 		ldapEmailSet := map[string]struct{}{}
 		for e := range flags {
 			ldapEmailSet[e] = struct{}{}
@@ -166,11 +172,35 @@ func (j *LdapSyncJob) Run() {
 			j.deleteClientsNotInLDAP(tag, ldapEmailSet)
 		}
 	}
+	j.lastFlagCount.Store(int64(len(flags)))
+}
+
+// FetchVlessFlags returns (empty, nil) when the bind succeeds but the search
+// yields nothing — a renamed OU, a lost read grant — which is not "all gone".
+func (j *LdapSyncJob) autoDeleteSafeForFetch(fetched int) bool {
+	if fetched == 0 {
+		logger.Warning("LDAP auto-delete skipped: directory returned no usable users")
+		return false
+	}
+	previous := j.lastFlagCount.Load()
+	if previous > 0 && int64(fetched)*100 < previous*ldapAutoDeleteMinRetainPercent {
+		logger.Warningf("LDAP auto-delete skipped: fetched %d users, previous successful sync saw %d (below %d%% retention)",
+			fetched, previous, ldapAutoDeleteMinRetainPercent)
+		return false
+	}
+	return true
+}
+
+func truthyValuesOrDefault(s string) []string {
+	if vals := splitCsv(s); len(vals) > 0 {
+		return vals
+	}
+	return DefaultTruthyValues
 }
 
 func splitCsv(s string) []string {
 	if s == "" {
-		return DefaultTruthyValues
+		return nil
 	}
 	parts := strings.Split(s, ",")
 	out := make([]string, 0, len(parts))