mirror of
https://github.com/MHSanaei/3x-ui.git
synced 2026-06-06 20:39:35 +00:00
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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user