From e409bc305d85cc8152d433067cd8adb8d7573cd3 Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sat, 6 Jun 2026 02:20:39 +0200 Subject: [PATCH] fix(iplimit): skip stale access-log emails after client rename/delete The IP-limit job scrapes the Xray access log, which keeps lines tagged with a client's old email for up to a log-rotation cycle after a rename or delete. For each such email getInboundByEmail (settings LIKE %email%) found nothing, so the job logged 'failed to fetch inbound settings: record not found' every run and recreated an inbound_client_ips row for the dead email (rows reappeared even after manual deletion). processLogFile now resolves the inbound once per email: if it maps to no inbound (gorm.ErrRecordNotFound) it logs at Debug, drops any orphan tracking row, and skips - so stale entries self-heal instead of spamming ERROR. The resolved inbound is passed into updateInboundClientIps, removing its internal lookup. updateClientTraffics also calls DelClientIPs alongside DelClientStat so a full inbound edit that drops an email doesn't leave a ghost row. Closes #4963 --- web/job/check_client_ip_job.go | 37 ++++++++++++++---- .../check_client_ip_job_integration_test.go | 38 ++++++++++++++++++- web/service/inbound.go | 5 +++ 3 files changed, 70 insertions(+), 10 deletions(-) diff --git a/web/job/check_client_ip_job.go b/web/job/check_client_ip_job.go index 991a312f..41f2484e 100644 --- a/web/job/check_client_ip_job.go +++ b/web/job/check_client_ip_job.go @@ -17,6 +17,8 @@ import ( "github.com/mhsanaei/3x-ui/v3/database/model" "github.com/mhsanaei/3x-ui/v3/logger" "github.com/mhsanaei/3x-ui/v3/xray" + + "gorm.io/gorm" ) // IPWithTimestamp tracks an IP address with its last seen timestamp @@ -184,6 +186,22 @@ func (j *CheckClientIpJob) processLogFile(enforce bool) bool { shouldCleanLog := false for email, ipTimestamps := range inboundClientIps { + // The access log can still reference a client that was just renamed + // or deleted; its email no longer matches any inbound. Skip it (and + // drop any orphaned tracking row) instead of recreating a row and + // logging an ERROR every run until the log rotates out the old email + // (#4963). + inbound, err := j.getInboundByEmail(email) + if err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + logger.Debugf("[LimitIP] skipping stale access-log email %q (renamed or deleted)", email) + j.delInboundClientIps(email) + } else { + j.checkError(err) + } + continue + } + // Convert to IPWithTimestamp slice ipsWithTime := make([]IPWithTimestamp, 0, len(ipTimestamps)) for ip, timestamp := range ipTimestamps { @@ -196,7 +214,7 @@ func (j *CheckClientIpJob) processLogFile(enforce bool) bool { continue } - shouldCleanLog = j.updateInboundClientIps(clientIpsRecord, email, ipsWithTime, enforce) || shouldCleanLog + shouldCleanLog = j.updateInboundClientIps(clientIpsRecord, inbound, email, ipsWithTime, enforce) || shouldCleanLog } return shouldCleanLog @@ -311,14 +329,17 @@ func (j *CheckClientIpJob) addInboundClientIps(clientEmail string, ipsWithTime [ return nil } -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 { - logger.Errorf("failed to fetch inbound settings for email %s: %s", clientEmail, err) - return false +// delInboundClientIps drops the inbound_client_ips tracking row for an email +// that no longer maps to any inbound (a renamed or deleted client), so stale +// access-log entries don't keep a ghost row alive (#4963). +func (j *CheckClientIpJob) delInboundClientIps(clientEmail string) { + db := database.GetDB() + if err := db.Where("client_email = ?", clientEmail).Delete(&model.InboundClientIps{}).Error; err != nil { + j.checkError(err) } +} +func (j *CheckClientIpJob) updateInboundClientIps(inboundClientIps *model.InboundClientIps, inbound *model.Inbound, clientEmail string, newIpsWithTime []IPWithTimestamp, enforce bool) bool { if inbound.Settings == "" { logger.Debug("wrong data:", inbound) return false @@ -411,7 +432,7 @@ func (j *CheckClientIpJob) updateInboundClientIps(inboundClientIps *model.Inboun inboundClientIps.Ips = string(jsonIps) db := database.GetDB() - err = db.Save(inboundClientIps).Error + err := db.Save(inboundClientIps).Error if err != nil { logger.Error("failed to save inboundClientIps:", err) return false diff --git a/web/job/check_client_ip_job_integration_test.go b/web/job/check_client_ip_job_integration_test.go index 4fed65af..ef28716f 100644 --- a/web/job/check_client_ip_job_integration_test.go +++ b/web/job/check_client_ip_job_integration_test.go @@ -195,7 +195,11 @@ func TestUpdateInboundClientIps_LiveIpNotBannedByStillFreshHistoricals(t *testin {IP: "128.71.1.1", Timestamp: now}, } - shouldCleanLog := j.updateInboundClientIps(row, email, live, true) + inbound, err := j.getInboundByEmail(email) + if err != nil { + t.Fatalf("getInboundByEmail: %v", err) + } + shouldCleanLog := j.updateInboundClientIps(row, inbound, 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 +248,11 @@ func TestUpdateInboundClientIps_ExcessLiveIpIsStillBanned(t *testing.T) { {IP: "192.0.2.9", Timestamp: now}, } - shouldCleanLog := j.updateInboundClientIps(row, email, live, true) + inbound, err := j.getInboundByEmail(email) + if err != nil { + t.Fatalf("getInboundByEmail: %v", err) + } + shouldCleanLog := j.updateInboundClientIps(row, inbound, email, live, true) if !shouldCleanLog { t.Fatalf("shouldCleanLog must be true when the live set exceeds the limit") @@ -321,6 +329,32 @@ func TestRun_CollectsIpsWithoutLimit(t *testing.T) { } } +// #4963: a stale access-log entry for a renamed/deleted client (its email no +// longer maps to any inbound) must not create or resurrect an +// inbound_client_ips row, and must drop any orphan left behind — instead of +// spamming "failed to fetch inbound settings" every run. +func TestRun_StaleAccessLogEmailIsSkippedAndOrphanDropped(t *testing.T) { + setupIntegrationDB(t) + t.Setenv("XUI_ENABLE_FAIL2BAN", "true") + fakeFail2BanClient(t) + + const staleEmail = "renamed-away" + // No inbound references staleEmail. Pre-seed an orphan tracking row to + // confirm the job removes it rather than leaving it to error forever. + seedClientIps(t, staleEmail, []IPWithTimestamp{{IP: "203.0.113.5", Timestamp: time.Now().Unix()}}) + writeXrayAccessLog(t, staleEmail, "203.0.113.5") + + NewCheckClientIpJob().Run() + + var count int64 + if err := database.GetDB().Model(&model.InboundClientIps{}).Where("client_email = ?", staleEmail).Count(&count).Error; err != nil { + t.Fatalf("count InboundClientIps: %v", err) + } + if count != 0 { + t.Fatalf("stale-email orphan row should be deleted, got %d row(s)", count) + } +} + // 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 diff --git a/web/service/inbound.go b/web/service/inbound.go index 670c370a..d89c81b2 100644 --- a/web/service/inbound.go +++ b/web/service/inbound.go @@ -1193,6 +1193,11 @@ func (s *InboundService) updateClientTraffics(tx *gorm.DB, oldInbound *model.Inb if err := s.DelClientStat(tx, email); err != nil { return err } + // Keep inbound_client_ips in sync when the inbound edit drops an + // email, so the IP-limit job doesn't keep a ghost tracking row (#4963). + if err := s.DelClientIPs(tx, email); err != nil { + return err + } } for i := range newClients { email := newClients[i].Email