feat(limitip): let operators exempt trusted addresses from the IP limit (#6230)

* feat(limitip): let operators exempt trusted addresses from the IP limit

Behind a shared address — an office gateway, a campus NAT, a residential
carrier — every user looks like the same client. One of them trips the IP
limit and the address is disconnected and handed to fail2ban, taking the
others with it. Today the only way out is editing jail.d by hand, which an
update overwrites.

Add an allowlist setting of addresses and networks. A matching address is
neither banned nor counted towards the limit: counting it would still cut the
shared network the entry exists to protect.

Entries are validated on save rather than skipped at scan time — a typo would
otherwise leave the address unprotected until someone noticed the bans.

* fix(limitip): keep each doc comment on its function and one grammar for the list

Three review follow-ups. loadAllowlist landed between hasLimitIp's doc comment
and hasLimitIp itself, so godoc showed one function's rationale above another's
body; it now sits after that function with its own comment.

The parser advertised semicolons and whitespace as separators while the
settings validator accepts commas only, making those forms unreachable through
the panel and the API — a promise the software never keeps. Both sides now read
the same comma-separated grammar.

The dist stub was a build artifact and does not belong in the tree.

* chore: drop the accidentally committed dist build stub

internal/web/dist/.gitkeep is what make dist-stub creates locally. Committing
it changes fresh-clone behaviour for everyone: today a bare go build fails
loudly on //go:embed all:dist, which is the documented signal to run the stub
target; with the file present the build succeeds and the panel serves an empty
dist instead.

* chore(i18n): translate the IP limit allowlist strings into the remaining locales

Ten locales carried the English source text verbatim; only ru-RU and uk-UA
were translated. The i18n dead-key test only checks that a key exists in every
file, so an untranslated value passes it silently.

Wording follows each locale's existing terms: the ipLimit noun already in the
file, and the comma-separated IP/CIDR phrasing from trustedProxyCidrsDesc.

* refactor(limitip): share one IP/CIDR list validator and read the allowlist only when enforcing

The allowlist check in CheckValid was a line-for-line copy of the trusted-proxy
loop directly above it. Both now call one helper, each passing its own message,
so the two lists cannot drift apart.

Run() read the allowlist on every 10s scan, including the majority of panels
where no client carries an IP limit and the value is discarded. It is now read
only once enforcement is known to apply.

CheckValid had no test for either list. The new one pins that a malformed entry
is rejected and that each list still names itself in the error, which is what
the shared helper could otherwise break.

---------

Co-authored-by: n0ctal <n0ctal@users.noreply.github.com>
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>

internal/web/job/check_client_ip_job.go

@@ -35,6 +35,7 @@ type CheckClientIpJob struct {
 	disAllowedIps []string
 	bannedSeen    map[string]int64
 	xrayService   service.XrayService
+	allowlist     ipLimitAllowlist
 }
 
 var job *CheckClientIpJob
@@ -67,7 +68,13 @@ func (j *CheckClientIpJob) Run() {
 	if hasLimit {
 		f2bInstalled = j.checkFail2BanInstalled()
 	}
-	j.processObserved(observed, j.resolveEnforce(hasLimit, f2bInstalled), true)
+	// Read only when the limit is actually applied: this runs every 10s and
+	// most panels carry no IP limit at all.
+	enforce := j.resolveEnforce(hasLimit, f2bInstalled)
+	if enforce {
+		j.allowlist = j.loadAllowlist()
+	}
+	j.processObserved(observed, enforce, true)
 }
 
 // resolveEnforce decides whether limits can actually be enforced this run.
@@ -126,6 +133,18 @@ func (j *CheckClientIpJob) hasLimitIp() bool {
 	return err == nil && probe > 0
 }
 
+// loadAllowlist reads the operator's trusted addresses once per scan; a bad
+// read leaves the list empty, which enforces the limit as before rather than
+// silently exempting everyone.
+func (j *CheckClientIpJob) loadAllowlist() ipLimitAllowlist {
+	raw, err := (&service.SettingService{}).GetIpLimitAllowlist()
+	if err != nil {
+		logger.Warning("[LimitIP] could not read the allowlist, enforcing without it:", err)
+		return ipLimitAllowlist{}
+	}
+	return parseIpLimitAllowlist(raw)
+}
+
 const ipScanChunk = 400
 
 func chunkEmails(s []string, size int) [][]string {
@@ -510,7 +529,11 @@ func (j *CheckClientIpJob) updateInboundClientIps(tx *gorm.DB, inboundClientIps
 	j.disAllowedIps = []string{}
 
 	// historical db-only ips are excluded from this count on purpose.
-	keptLive, bannedLive := selectIpsToBan(liveIps, limitIp)
+	limitedIps, allowedIps := j.allowlist.split(liveIps)
+	keptLive, bannedLive := selectIpsToBan(limitedIps, limitIp)
+	// Allowlisted addresses stay connected and out of the count: charging them
+	// against the limit would still cut the shared network the entry protects.
+	keptLive = append(keptLive, allowedIps...)
 	actionable := j.filterAdvancedSinceLastBan(clientEmail, bannedLive)
 	if len(actionable) > 0 {
 		shouldCleanLog = true

internal/web/job/check_client_ip_job_integration_test.go

@@ -419,3 +419,52 @@ func TestHasLimitIp_ProbesClientRecords(t *testing.T) {
 		t.Fatal("hasLimitIp = false with a limit_ip=2 client present")
 	}
 }
+
+// The mirror of TestUpdateInboundClientIps_ExcessLiveIpIsStillBanned: with the
+// older address on the operator's allowlist nothing may be banned, it must not
+// consume the limit, and no fail2ban line may be written for it (#5378).
+func TestUpdateInboundClientIps_AllowlistedIpIsNeitherCountedNorBanned(t *testing.T) {
+	setupIntegrationDB(t)
+
+	const email = "issue5378-office"
+	seedInboundWithClient(t, "inbound-issue5378", email, 1)
+
+	now := time.Now().Unix()
+	row := seedClientIps(t, email, []IPWithTimestamp{
+		{IP: "203.0.113.10", Timestamp: now - 60},
+	})
+
+	j := NewCheckClientIpJob()
+	j.allowlist = parseIpLimitAllowlist("203.0.113.0/24")
+
+	live := []IPWithTimestamp{
+		{IP: "203.0.113.10", Timestamp: now - 5},
+		{IP: "192.0.2.9", Timestamp: now},
+	}
+
+	inbound, err := j.getInboundByEmail(email)
+	if err != nil {
+		t.Fatalf("getInboundByEmail: %v", err)
+	}
+	_, banned := j.updateInboundClientIps(database.GetDB(), row, inbound, email, 1, live, true, false)
+
+	if banned {
+		t.Fatal("an allowlisted address pushed the client over its limit and something was banned")
+	}
+	if len(j.disAllowedIps) != 0 {
+		t.Fatalf("disAllowedIps = %v, want none", j.disAllowedIps)
+	}
+
+	persisted := ipSet(readClientIps(t, email))
+	for _, ip := range []string{"203.0.113.10", "192.0.2.9"} {
+		if _, ok := persisted[ip]; !ok {
+			t.Errorf("%s must still be persisted; got %v", ip, persisted)
+		}
+	}
+
+	if body, err := os.ReadFile(readIpLimitLogPath()); err == nil {
+		if contains(string(body), "203.0.113.10") {
+			t.Fatalf("an allowlisted address reached the fail2ban log:\n%s", body)
+		}
+	}
+}

internal/web/job/ip_limit_allowlist.go

@@ -0,0 +1,80 @@
+package job
+
+import (
+	"net/netip"
+	"strings"
+)
+
+// ipLimitAllowlist holds the operator's trusted addresses and networks. An IP
+// that matches is neither counted towards a client's IP limit nor banned:
+// counting it would still cut the office or campus NAT the entry exists to
+// protect, which is the whole point of the setting (#5378).
+type ipLimitAllowlist struct {
+	prefixes []netip.Prefix
+	addrs    []netip.Addr
+}
+
+// parseIpLimitAllowlist reads the comma-separated form the settings validator
+// enforces, each entry either a CIDR or a bare address. Entries that do not
+// parse are skipped rather than failing the scan: the validator rejects them on
+// save, so anything reaching here is either valid or a hand-edited database.
+func parseIpLimitAllowlist(raw string) ipLimitAllowlist {
+	var list ipLimitAllowlist
+	for _, field := range strings.Split(raw, ",") {
+		field = strings.TrimSpace(field)
+		if field == "" {
+			continue
+		}
+		if prefix, err := netip.ParsePrefix(field); err == nil {
+			list.prefixes = append(list.prefixes, prefix.Masked())
+			continue
+		}
+		if addr, err := netip.ParseAddr(field); err == nil {
+			list.addrs = append(list.addrs, addr.Unmap())
+		}
+	}
+	return list
+}
+
+func (l ipLimitAllowlist) empty() bool {
+	return len(l.prefixes) == 0 && len(l.addrs) == 0
+}
+
+func (l ipLimitAllowlist) contains(ip string) bool {
+	if l.empty() {
+		return false
+	}
+	addr, err := netip.ParseAddr(strings.TrimSpace(ip))
+	if err != nil {
+		return false
+	}
+	addr = addr.Unmap()
+	for _, allowed := range l.addrs {
+		if allowed == addr {
+			return true
+		}
+	}
+	for _, prefix := range l.prefixes {
+		if prefix.Contains(addr) {
+			return true
+		}
+	}
+	return false
+}
+
+// split separates the entries an allowlist protects from the ones the limit
+// still applies to, preserving the caller's ordering in both.
+func (l ipLimitAllowlist) split(entries []IPWithTimestamp) (limited, allowed []IPWithTimestamp) {
+	if l.empty() {
+		return entries, nil
+	}
+	limited = make([]IPWithTimestamp, 0, len(entries))
+	for _, entry := range entries {
+		if l.contains(entry.IP) {
+			allowed = append(allowed, entry)
+			continue
+		}
+		limited = append(limited, entry)
+	}
+	return limited, allowed
+}

internal/web/job/ip_limit_allowlist_test.go

@@ -0,0 +1,70 @@
+package job
+
+import "testing"
+
+// Addresses in the examples below come from the documentation ranges reserved
+// by RFC 5737 and RFC 3849.
+func TestIpLimitAllowlistMatchesAddressesAndNetworks(t *testing.T) {
+	list := parseIpLimitAllowlist("203.0.113.10, 198.51.100.0/24 , 2001:db8::/32, not-an-ip")
+
+	for _, ip := range []string{"203.0.113.10", "198.51.100.7", "2001:db8::1"} {
+		if !list.contains(ip) {
+			t.Fatalf("%s should be allowlisted", ip)
+		}
+	}
+	for _, ip := range []string{"203.0.113.11", "192.0.2.5", "2001:db9::1", ""} {
+		if list.contains(ip) {
+			t.Fatalf("%s must not be allowlisted", ip)
+		}
+	}
+}
+
+// A typo must not disable the limit for everybody, so an unparsable entry is
+// dropped and the rest of the list keeps working.
+func TestIpLimitAllowlistIgnoresUnparsableEntries(t *testing.T) {
+	list := parseIpLimitAllowlist("nonsense, 203.0.113.0/24")
+	if !list.contains("203.0.113.5") {
+		t.Fatal("a valid entry stopped working because a neighbouring one was malformed")
+	}
+	if list.contains("192.0.2.1") {
+		t.Fatal("a malformed entry must not widen the allowlist")
+	}
+	if parseIpLimitAllowlist("nonsense").empty() != true {
+		t.Fatal("a list of only malformed entries must be empty, not permissive")
+	}
+}
+
+// The point of the setting: a shared address is neither banned nor counted, so
+// the office NAT it protects does not consume the client's limit either.
+func TestIpLimitAllowlistSplitKeepsAllowedOutOfTheCount(t *testing.T) {
+	live := []IPWithTimestamp{
+		{IP: "203.0.113.10", Timestamp: 1},
+		{IP: "192.0.2.1", Timestamp: 2},
+		{IP: "192.0.2.2", Timestamp: 3},
+	}
+	list := parseIpLimitAllowlist("203.0.113.10")
+
+	limited, allowed := list.split(live)
+	if len(allowed) != 1 || allowed[0].IP != "203.0.113.10" {
+		t.Fatalf("allowed = %v, want the allowlisted address alone", allowed)
+	}
+	if len(limited) != 2 {
+		t.Fatalf("limited = %v, want the two ordinary addresses", limited)
+	}
+
+	kept, banned := selectIpsToBan(limited, 2)
+	if len(banned) != 0 {
+		t.Fatalf("banned = %v, want none: the allowlisted address must not push an ordinary one over the limit", banned)
+	}
+	if len(kept) != 2 {
+		t.Fatalf("kept = %v, want both ordinary addresses", kept)
+	}
+}
+
+func TestIpLimitAllowlistEmptyListChangesNothing(t *testing.T) {
+	live := []IPWithTimestamp{{IP: "192.0.2.1", Timestamp: 1}, {IP: "192.0.2.2", Timestamp: 2}}
+	limited, allowed := parseIpLimitAllowlist("").split(live)
+	if allowed != nil || len(limited) != 2 {
+		t.Fatalf("empty allowlist changed the input: limited=%v allowed=%v", limited, allowed)
+	}
+}