From 66d4d047768cf1f59538b6176a4dc5e0537a3d6d Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Tue, 2 Jun 2026 14:43:11 +0200 Subject: [PATCH] fix(iplimit): populate client IP log without an IP limit The per-client IP log was only filled as a side effect of IP-limit enforcement: Run() scraped the access log only when some client had limitIp>0, so installs without a limit always showed an empty IP log (#4800). Decouple collection from enforcement: scrape the access log whenever it is available and thread an enforce flag through processLogFile/updateInboundClientIps so banning still only happens for limited clients. The XUI_ENABLE_FAIL2BAN kill-switch is preserved. Closes #4800 --- web/job/check_client_ip_job.go | 78 ++++--------------- .../check_client_ip_job_integration_test.go | 53 ++++++++++++- 2 files changed, 66 insertions(+), 65 deletions(-) diff --git a/web/job/check_client_ip_job.go b/web/job/check_client_ip_job.go index b7d0efa4..991a312f 100644 --- a/web/job/check_client_ip_job.go +++ b/web/job/check_client_ip_job.go @@ -35,23 +35,6 @@ var job *CheckClientIpJob const defaultXrayAPIPort = 62789 -// ipStaleAfterSeconds controls how long a client IP kept in the -// per-client tracking table (model.InboundClientIps.Ips) is considered -// still "active" before it's evicted during the next scan. -// -// Without this eviction, an IP that connected once and then went away -// keeps sitting in the table with its old timestamp. Because the -// excess-IP selector sorts ascending ("newest wins, oldest loses") to -// protect the most recent connections, that stale entry keeps -// occupying a slot and the IP that is *actually* currently using the -// config gets classified as "new excess" and banned by fail2ban on -// every single run — producing the continuous ban loop from #4077. -// -// 30 minutes is chosen so an actively-streaming client (where xray -// emits a fresh `accepted` log line whenever it opens a new TCP) will -// always refresh its timestamp well within the window, but a client -// that has really stopped using the config will drop out of the table -// in a bounded time and free its slot. const ipStaleAfterSeconds = int64(30 * 60) // NewCheckClientIpJob creates a new client IP monitoring job instance. @@ -67,27 +50,20 @@ func (j *CheckClientIpJob) Run() { shouldClearAccessLog := false fail2BanEnabled := isFail2BanEnabled() - iplimitActive := fail2BanEnabled && j.hasLimitIp() + hasLimit := fail2BanEnabled && j.hasLimitIp() f2bInstalled := false - if iplimitActive { + if hasLimit { f2bInstalled = j.checkFail2BanInstalled() } - isAccessLogAvailable := j.checkAccessLogAvailable(iplimitActive) + isAccessLogAvailable := j.checkAccessLogAvailable(hasLimit) - if isAccessLogAvailable { - if runtime.GOOS == "windows" { - if iplimitActive { - shouldClearAccessLog = j.processLogFile() - } - } else { - if iplimitActive { - if f2bInstalled { - shouldClearAccessLog = j.processLogFile() - } else { - logger.Warning("[LimitIP] Fail2Ban is not installed, Please install Fail2Ban from the x-ui bash menu.") - } - } + if fail2BanEnabled && isAccessLogAvailable { + enforce := hasLimit + if hasLimit && runtime.GOOS != "windows" && !f2bInstalled { + logger.Warning("[LimitIP] Fail2Ban is not installed, Please install Fail2Ban from the x-ui bash menu.") + enforce = false } + shouldClearAccessLog = j.processLogFile(enforce) } if shouldClearAccessLog || (isAccessLogAvailable && time.Now().Unix()-j.lastClear > 3600) { @@ -145,7 +121,7 @@ func (j *CheckClientIpJob) hasLimitIp() bool { return false } -func (j *CheckClientIpJob) processLogFile() bool { +func (j *CheckClientIpJob) processLogFile(enforce bool) bool { ipRegex := regexp.MustCompile(`from (?:tcp:|udp:)?\[?([0-9a-fA-F\.:]+)\]?:\d+ accepted`) emailRegex := regexp.MustCompile(`email: (.+)$`) @@ -220,18 +196,12 @@ func (j *CheckClientIpJob) processLogFile() bool { continue } - shouldCleanLog = j.updateInboundClientIps(clientIpsRecord, email, ipsWithTime) || shouldCleanLog + shouldCleanLog = j.updateInboundClientIps(clientIpsRecord, email, ipsWithTime, enforce) || shouldCleanLog } return shouldCleanLog } -// mergeClientIps combines the persisted (old) and freshly observed (new) -// IP-with-timestamp lists for a single client into a map. An entry is -// dropped if its last-seen timestamp is older than staleCutoff. -// -// Extracted as a helper so updateInboundClientIps can stay DB-oriented -// and the merge policy can be exercised by a unit test. func mergeClientIps(old, new []IPWithTimestamp, staleCutoff int64) map[string]int64 { ipMap := make(map[string]int64, len(old)+len(new)) for _, ipTime := range old { @@ -251,19 +221,6 @@ func mergeClientIps(old, new []IPWithTimestamp, staleCutoff int64) map[string]in return ipMap } -// partitionLiveIps splits the merged ip map into live (seen in the -// current scan) and historical (only in the db blob, still inside the -// staleness window). -// -// only live ips count toward the per-client limit. historical ones stay -// in the db so the panel keeps showing them, but they must not take a -// live slot or get re-banned. the 30min cutoff alone isn't tight enough: -// an ip that stopped connecting a few minutes ago still looks fresh to -// mergeClientIps, and without this split it would keep triggering -// fail2ban even though it isn't currently connected. see #4077 / #4091. -// -// live is sorted ascending by timestamp (oldest → newest), so we keep -// the most recent entries at the end of the slice (last IP wins). func partitionLiveIps(ipMap map[string]int64, observedThisScan map[string]bool) (live, historical []IPWithTimestamp) { live = make([]IPWithTimestamp, 0, len(observedThisScan)) historical = make([]IPWithTimestamp, 0, len(ipMap)) @@ -354,7 +311,7 @@ func (j *CheckClientIpJob) addInboundClientIps(clientEmail string, ipsWithTime [ return nil } -func (j *CheckClientIpJob) updateInboundClientIps(inboundClientIps *model.InboundClientIps, clientEmail string, newIpsWithTime []IPWithTimestamp) bool { +func (j *CheckClientIpJob) updateInboundClientIps(inboundClientIps *model.InboundClientIps, clientEmail string, newIpsWithTime []IPWithTimestamp, enforce bool) bool { // Get the inbound configuration inbound, err := j.getInboundByEmail(clientEmail) if err != nil { @@ -382,8 +339,9 @@ func (j *CheckClientIpJob) updateInboundClientIps(inboundClientIps *model.Inboun } } - if !clientFound || limitIp <= 0 || !inbound.Enable { - // No limit or inbound disabled, just update and return + if !enforce || !clientFound || limitIp <= 0 || !inbound.Enable { + // Nothing to enforce (collection-only run, no limit, client missing, or + // inbound disabled): record the observed IPs for the panel and return. jsonIps, _ := json.Marshal(newIpsWithTime) inboundClientIps.Ips = string(jsonIps) db := database.GetDB() @@ -397,8 +355,6 @@ func (j *CheckClientIpJob) updateInboundClientIps(inboundClientIps *model.Inboun json.Unmarshal([]byte(inboundClientIps.Ips), &oldIpsWithTime) } - // Merge old and new IPs, evicting entries that haven't been - // re-observed in a while. See mergeClientIps / #4077 for why. ipMap := mergeClientIps(oldIpsWithTime, newIpsWithTime, time.Now().Unix()-ipStaleAfterSeconds) // only ips seen in this scan count toward the limit. see @@ -422,10 +378,6 @@ func (j *CheckClientIpJob) updateInboundClientIps(inboundClientIps *model.Inboun keptLive = liveIps[cutoff:] bannedLive := liveIps[:cutoff] - // Open log file only when a ban entry needs to be written. - // Use a local logger to avoid mutating the global log.* state, - // which would redirect all standard-library logging to this file - // and leave a dangling closed-file handle after the defer fires. logIpFile, err := os.OpenFile(xray.GetIPLimitLogPath(), os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0644) if err != nil { logger.Errorf("failed to open IP limit log file: %s", err) diff --git a/web/job/check_client_ip_job_integration_test.go b/web/job/check_client_ip_job_integration_test.go index de7e857c..4fed65af 100644 --- a/web/job/check_client_ip_job_integration_test.go +++ b/web/job/check_client_ip_job_integration_test.go @@ -195,7 +195,7 @@ func TestUpdateInboundClientIps_LiveIpNotBannedByStillFreshHistoricals(t *testin {IP: "128.71.1.1", Timestamp: now}, } - shouldCleanLog := j.updateInboundClientIps(row, email, live) + shouldCleanLog := j.updateInboundClientIps(row, email, live, true) if shouldCleanLog { t.Fatalf("shouldCleanLog must be false, nothing should have been banned with 1 live ip under limit 3") @@ -244,7 +244,7 @@ func TestUpdateInboundClientIps_ExcessLiveIpIsStillBanned(t *testing.T) { {IP: "192.0.2.9", Timestamp: now}, } - shouldCleanLog := j.updateInboundClientIps(row, email, live) + shouldCleanLog := j.updateInboundClientIps(row, email, live, true) if !shouldCleanLog { t.Fatalf("shouldCleanLog must be true when the live set exceeds the limit") @@ -272,6 +272,55 @@ func TestUpdateInboundClientIps_ExcessLiveIpIsStillBanned(t *testing.T) { } } +// writeXrayAccessLog points bin/config.json at a fresh access.log holding a +// single default-format Xray line (`from tcp:: accepted … email: `) +// for the given client, so Run() has something to scrape. +func writeXrayAccessLog(t *testing.T, email, ip string) { + t.Helper() + binDir := t.TempDir() + accessLog := filepath.Join(t.TempDir(), "access.log") + t.Setenv("XUI_BIN_FOLDER", binDir) + configData, err := json.Marshal(map[string]any{ + "log": map[string]any{"access": accessLog}, + }) + if err != nil { + t.Fatalf("marshal xray config: %v", err) + } + if err := os.WriteFile(filepath.Join(binDir, "config.json"), configData, 0644); err != nil { + t.Fatalf("write xray config: %v", err) + } + line := "2026/06/02 13:35:53 from tcp:" + ip + ":2387 accepted tcp:example.com:443 email: " + email + "\n" + if err := os.WriteFile(accessLog, []byte(line), 0644); err != nil { + t.Fatalf("write access log: %v", err) + } +} + +// #4800: the per-client IP log must populate even when no client has an IP +// limit. Before the fix, Run() only scraped the access log when an IP limit +// was active, so a limit-free install always showed an empty IP log despite +// valid access-log lines. No ban may be written since there's no limit. +func TestRun_CollectsIpsWithoutLimit(t *testing.T) { + setupIntegrationDB(t) + t.Setenv("XUI_ENABLE_FAIL2BAN", "true") + fakeFail2BanClient(t) + + const email = "no-limit-user" + seedInboundWithClient(t, "inbound-no-limit", email, 0) // limitIp = 0 + writeXrayAccessLog(t, email, "203.0.113.10") + + NewCheckClientIpJob().Run() + + ips := readClientIps(t, email) + if len(ips) != 1 || ips[0].IP != "203.0.113.10" { + t.Fatalf("expected the access-log IP to be collected without a limit, got %v", ips) + } + + if info, err := os.Stat(readIpLimitLogPath()); err == nil && info.Size() > 0 { + body, _ := os.ReadFile(readIpLimitLogPath()) + t.Fatalf("3xipl.log should be empty with no limit set, got:\n%s", body) + } +} + // readIpLimitLogPath reads the 3xipl.log path the same way the job // does via xray.GetIPLimitLogPath but without importing xray here // just for the path helper (which would pull a lot more deps into the