From 3f1dd4bf5a1288fd8bd55d770217805895f477e7 Mon Sep 17 00:00:00 2001 From: n0ctal <4c866w5fn9@privaterelay.appleid.com> Date: Tue, 18 Aug 2026 18:24:59 +0500 Subject: [PATCH] fix: follow-ups from the post-merge reviews of #6221, #6227, #6230 and #6239 (#6250) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: follow-ups from the post-merge reviews of #6221, #6227, #6230 and #6239 Six defects the automated reviews found after those PRs merged. Each is verified rather than taken on trust — two by experiment, the rest by reading the merged code. **Import restore never wrote an empty local value** (#6227). GORM builds the assignment map from the struct passed to Assign and drops zero-valued fields, so `Assign(model.Setting{Value: ""})` produced an empty Updates and the imported row survived. Empty is the normal state: UpdateAllSetting writes a row for every AllSetting field including the blank ones. That is exactly the case the PR existed for — a destination with no certificate inheriting the source machine's path. Confirmed with a throwaway test before changing anything: the value stayed "IMPORTED". Now uses saveSetting, which is not zero-filtered. **Import destroyed node mTLS material** (#6227). The "no local row means the default applied, so drop the import" branch fires for the five nodeMtls* keys, which are minted on demand and deliberately absent from AllSetting, so a fresh install has no row for them. Reinstall-then-restore therefore deleted the CA certificate and its private key — and the backup was the only copy, since neither is surfaced in the UI or the export. Those keys are now kept. **The clients-list enable toggle wiped renewal state** (#6239, #6238). setEnable hand-builds the update payload and carried reset but not resetDay or resetMax, so one click on the switch turned calendar mode off and lifted the renewal cap permanently. The form-modal tests could not catch it because that path does send both fields. **"Delete depleted clients" deleted calendar clients** (#6239). The predicate read `reset = 0` as "does not auto-renew", which is exactly the calendar shape, in two places. Both now share one constant that also requires `reset_day = 0`. **Allowlist validation and parsing disagreed** (#6230). Save used net, scan used netip, and they differ: `198.51.100.0/024` saves without complaint and is silently dropped at scan — the failure the PR set out to remove. Verified by running both parsers. An IPv4-mapped prefix parsed but could never match, because contains() unmaps the query while the prefix stayed 128-bit; it is unmapped at parse now. A test asserts the two acceptance sets agree. **A comment stated the opposite of the truth** (#6221). GetInbounds has no enable filter, so a node reports a disabled inbound normally; the row in that bug report was missing only because it was never delivered. Reworded to the real invariant. Also trims two comment blocks in ip_limit_allowlist.go to the repo's two-line maximum. Not included: the reviewer's suggestion to lift the node hand-off out of `if inbound.Enable` in AddInbound. It is the right root-cause fix, but it changes delivery behaviour on multi-node deployments and belongs in its own change with its own testing, not in a cleanup batch. One reported finding is not real: BulkCreate does call validateClientResetDay, validateClientResetMax and validateClientTrafficReset — verified in the merged tree. * fix(netsafe): wrap both errors so errorlint passes Unrelated to this PR's subject and in a file it does not otherwise touch. It is here only because CI lints the merge result, and `main` has been red since #6242 landed: `fmt.Errorf("%w; %v", ...)` wraps the first error and formats the second, which errorlint rejects. Go 1.20 allows more than one %w, so both are wrapped now and `errors.Is` works against either. --- frontend/src/hooks/useClients.ts | 2 + internal/util/netsafe/netsafe.go | 2 +- internal/web/entity/entity.go | 21 ++++++- internal/web/job/ip_limit_allowlist.go | 19 +++--- .../job/ip_limit_allowlist_agreement_test.go | 36 +++++++++++ internal/web/service/client_bulk.go | 2 +- .../web/service/depleted_calendar_test.go | 17 ++++++ .../web/service/import_host_settings_test.go | 60 +++++++++++++++++-- internal/web/service/inbound_node.go | 5 +- internal/web/service/inbound_traffic.go | 6 +- internal/web/service/server.go | 26 +++++--- 11 files changed, 169 insertions(+), 27 deletions(-) create mode 100644 internal/web/job/ip_limit_allowlist_agreement_test.go create mode 100644 internal/web/service/depleted_calendar_test.go diff --git a/frontend/src/hooks/useClients.ts b/frontend/src/hooks/useClients.ts index 15403ae62..87fd8d7de 100644 --- a/frontend/src/hooks/useClients.ts +++ b/frontend/src/hooks/useClients.ts @@ -539,6 +539,8 @@ export function useClients(options: UseClientsOptions = {}) { limitHwid: base.limitHwid || 0, tgId: Number(base.tgId) || 0, reset: Number(base.reset) || 0, + resetDay: Number(base.resetDay) || 0, + resetMax: Number(base.resetMax) || 0, group: base.group || '', comment: base.comment || '', enable: !!enable, diff --git a/internal/util/netsafe/netsafe.go b/internal/util/netsafe/netsafe.go index c43f2f0c3..3122940e4 100644 --- a/internal/util/netsafe/netsafe.go +++ b/internal/util/netsafe/netsafe.go @@ -64,7 +64,7 @@ func SSRFGuardedDialContext(ctx context.Context, network, addr string) (net.Conn // refusal is reported alongside instead of being lost to the last failure. if blockedErr != nil { if lastErr != nil { - return nil, fmt.Errorf("%w; %v", blockedErr, lastErr) + return nil, fmt.Errorf("%w; %w", blockedErr, lastErr) } return nil, blockedErr } diff --git a/internal/web/entity/entity.go b/internal/web/entity/entity.go index 391c0269f..159a0d608 100644 --- a/internal/web/entity/entity.go +++ b/internal/web/entity/entity.go @@ -5,6 +5,7 @@ import ( "math" "net" "net/mail" + "net/netip" "strings" "time" @@ -153,6 +154,24 @@ func pathHasForbiddenChar(s string) bool { return false } +// CheckNetipAddrOrPrefixList mirrors parseIpLimitAllowlist exactly: net and netip +// disagree (net accepts "/024", netip does not), so save and scan must share rules. +func CheckNetipAddrOrPrefixList(list, message string) error { + for entry := range strings.SplitSeq(list, ",") { + entry = strings.TrimSpace(entry) + if entry == "" { + continue + } + if _, err := netip.ParseAddr(entry); err == nil { + continue + } + if _, err := netip.ParsePrefix(entry); err != nil { + return common.NewError(message, entry) + } + } + return nil +} + // checkIPOrCIDRList rejects the first comma-separated entry that is neither a // bare address nor a CIDR, naming it with the caller's message. func checkIPOrCIDRList(list, message string) error { @@ -259,7 +278,7 @@ func (s *AllSetting) CheckValid() error { // Rejected here rather than skipped at scan time: a typo in an allowlist // entry silently leaves the address unprotected until a trusted network gets banned. - if err := checkIPOrCIDRList(s.IpLimitAllowlist, "IP limit allowlist entry is not valid:"); err != nil { + if err := CheckNetipAddrOrPrefixList(s.IpLimitAllowlist, "IP limit allowlist entry is not valid:"); err != nil { return err } diff --git a/internal/web/job/ip_limit_allowlist.go b/internal/web/job/ip_limit_allowlist.go index cd7cc489b..a9f3475ea 100644 --- a/internal/web/job/ip_limit_allowlist.go +++ b/internal/web/job/ip_limit_allowlist.go @@ -5,19 +5,15 @@ import ( "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). +// An address that matches is neither counted towards a client's IP limit nor +// banned: counting it would still cut the shared network it protects (#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. +// Comma-separated, each entry a CIDR or a bare address. Unparseable entries are +// skipped: the validator uses these same rules, so only a hand-edited DB differs. func parseIpLimitAllowlist(raw string) ipLimitAllowlist { var list ipLimitAllowlist for _, field := range strings.Split(raw, ",") { @@ -26,6 +22,13 @@ func parseIpLimitAllowlist(raw string) ipLimitAllowlist { continue } if prefix, err := netip.ParsePrefix(field); err == nil { + // Unmapped: contains() unmaps the queried address, and Prefix.Contains + // is false whenever the bit lengths disagree. + if addr := prefix.Addr(); addr.Is4In6() { + if p4, perr := addr.Unmap().Prefix(prefix.Bits() - 96); perr == nil { + prefix = p4 + } + } list.prefixes = append(list.prefixes, prefix.Masked()) continue } diff --git a/internal/web/job/ip_limit_allowlist_agreement_test.go b/internal/web/job/ip_limit_allowlist_agreement_test.go new file mode 100644 index 000000000..106b24ddf --- /dev/null +++ b/internal/web/job/ip_limit_allowlist_agreement_test.go @@ -0,0 +1,36 @@ +package job + +import ( + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/web/entity" +) + +// Save-time validation and scan-time parsing must accept exactly the same set: +// anything the validator lets through and the parser drops is silently unprotected. +func TestAllowlistValidatorAndParserAgree(t *testing.T) { + for _, entry := range []string{ + "198.51.100.7", + "198.51.100.0/24", + "2001:db8::1", + "2001:db8::/32", + "198.51.100.0/024", + "not-an-address", + "198.51.100.0/33", + } { + accepted := entity.CheckNetipAddrOrPrefixList(entry, "invalid:") == nil + parsed := len(parseIpLimitAllowlist(entry).prefixes)+len(parseIpLimitAllowlist(entry).addrs) > 0 + if accepted != parsed { + t.Errorf("%q: validator=%v parser=%v — a disagreement leaves the entry silently unprotected", entry, accepted, parsed) + } + } +} + +// An IPv4-mapped prefix used to parse but never match, because contains() unmaps +// the queried address and Prefix.Contains is false across bit lengths. +func TestAllowlistMatchesIPv4MappedPrefix(t *testing.T) { + list := parseIpLimitAllowlist("::ffff:198.51.100.0/120") + if !list.contains("198.51.100.5") { + t.Fatal("an IPv4-mapped entry matched nothing: it protects no one") + } +} diff --git a/internal/web/service/client_bulk.go b/internal/web/service/client_bulk.go index cd04ab58a..e87447594 100644 --- a/internal/web/service/client_bulk.go +++ b/internal/web/service/client_bulk.go @@ -1336,7 +1336,7 @@ func (s *ClientService) BulkCreate(inboundSvc *InboundService, payloads []Client func (s *ClientService) DelDepleted(inboundSvc *InboundService) (int, bool, error) { db := database.GetDB() now := time.Now().UnixMilli() - depletedClause := "reset = 0 and ((total > 0 and up + down >= total) or (expiry_time > 0 and expiry_time <= ?))" + depletedClause := depletedClientsClause var rows []xray.ClientTraffic if err := db.Where(depletedClause, now).Find(&rows).Error; err != nil { diff --git a/internal/web/service/depleted_calendar_test.go b/internal/web/service/depleted_calendar_test.go new file mode 100644 index 000000000..56edeb484 --- /dev/null +++ b/internal/web/service/depleted_calendar_test.go @@ -0,0 +1,17 @@ +package service + +import ( + "strings" + "testing" +) + +// A calendar client has reset = 0, so the old predicate called it depleted at all +// times and the operator's purge deleted it along with its traffic row (#6239). +func TestDepletedClauseExcludesCalendarClients(t *testing.T) { + if !strings.Contains(depletedClientsClause, "reset_day = 0") { + t.Fatalf("predicate ignores reset_day, so a calendar client would be purged: %q", depletedClientsClause) + } + if !strings.Contains(depletedClientsClause, "reset = 0") { + t.Fatalf("predicate no longer protects interval clients: %q", depletedClientsClause) + } +} diff --git a/internal/web/service/import_host_settings_test.go b/internal/web/service/import_host_settings_test.go index 9228e508c..dd3f4de14 100644 --- a/internal/web/service/import_host_settings_test.go +++ b/internal/web/service/import_host_settings_test.go @@ -70,16 +70,15 @@ func TestImportKeepsHostBoundSettings(t *testing.T) { } } -// The destination usually has no row at all for the certificate paths and the -// node identity — the built-in default applies. The imported row must go, or -// the panel quietly adopts the source machine's certificate path. +// A certificate path this machine never set must not be inherited from the +// source. Lazily minted material is the opposite case and is covered below. func TestImportDropsHostBoundSettingsThisMachineNeverHad(t *testing.T) { setupConflictDB(t) db := database.GetDB() kept := captureHostBoundSettings() - for _, key := range []string{"webCertFile", "subCertFile", "nodeMtlsClientCertPem"} { + for _, key := range []string{"webCertFile", "subCertFile"} { if err := db.Create(&model.Setting{Key: key, Value: "from-imported-file"}).Error; err != nil { t.Fatalf("seed imported %s: %v", key, err) } @@ -87,7 +86,7 @@ func TestImportDropsHostBoundSettingsThisMachineNeverHad(t *testing.T) { restoreHostBoundSettings(kept) - for _, key := range []string{"webCertFile", "subCertFile", "nodeMtlsClientCertPem"} { + for _, key := range []string{"webCertFile", "subCertFile"} { var count int64 if err := db.Model(&model.Setting{}).Where("key = ?", key).Count(&count).Error; err != nil { t.Fatal(err) @@ -97,3 +96,54 @@ func TestImportDropsHostBoundSettingsThisMachineNeverHad(t *testing.T) { } } } + +// Node mTLS material is minted on demand, so a fresh install has no row and the +// imported copy is the only one there is — including the CA private key (#6227). +func TestImportKeepsLazilyMintedMaterialThisMachineNeverHad(t *testing.T) { + setupConflictDB(t) + db := database.GetDB() + + kept := captureHostBoundSettings() + + for _, key := range []string{"nodeMtlsCaCertPem", "nodeMtlsCaKeyPem", "nodeMtlsClientCertPem", "nodeMtlsClientKeyPem", "nodeMtlsClientCAPem"} { + if err := db.Create(&model.Setting{Key: key, Value: "from-imported-file"}).Error; err != nil { + t.Fatalf("seed imported %s: %v", key, err) + } + } + + restoreHostBoundSettings(kept) + + for _, key := range []string{"nodeMtlsCaCertPem", "nodeMtlsCaKeyPem", "nodeMtlsClientCertPem", "nodeMtlsClientKeyPem", "nodeMtlsClientCAPem"} { + var got model.Setting + if err := db.Where("key = ?", key).First(&got).Error; err != nil { + t.Fatalf("%s was dropped; restoring a backup onto a reinstalled panel would lose it: %v", key, err) + } + } +} + +// An empty local value is the normal state once Panel Settings has been saved: +// GORM's Assign(struct) dropped it, so the source machine's path survived. +func TestImportRestoresEmptyLocalValueOverImported(t *testing.T) { + setupConflictDB(t) + db := database.GetDB() + + if err := db.Create(&model.Setting{Key: "webCertFile", Value: ""}).Error; err != nil { + t.Fatalf("seed local empty: %v", err) + } + kept := captureHostBoundSettings() + + if err := db.Model(&model.Setting{}).Where("key = ?", "webCertFile"). + Update("value", "/etc/ssl/source-host.pem").Error; err != nil { + t.Fatalf("seed imported: %v", err) + } + + restoreHostBoundSettings(kept) + + var got model.Setting + if err := db.Where("key = ?", "webCertFile").First(&got).Error; err != nil { + t.Fatal(err) + } + if got.Value != "" { + t.Fatalf("webCertFile = %q, want the empty local value back: the panel still points at the source machine's certificate", got.Value) + } +} diff --git a/internal/web/service/inbound_node.go b/internal/web/service/inbound_node.go index 71e5d3910..44909ff4f 100644 --- a/internal/web/service/inbound_node.go +++ b/internal/web/service/inbound_node.go @@ -714,9 +714,8 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi if dirty { continue } - // Disabled inbounds are intentionally absent from the node's runtime - // snapshot. Their absence is not evidence of deletion; retain the row, - // client history and port reservation until an explicit delete occurs. + // A node inbound created disabled is never delivered, so its absence from + // the snapshot is ambiguous rather than evidence of a node-side delete. if !c.Enable { continue } diff --git a/internal/web/service/inbound_traffic.go b/internal/web/service/inbound_traffic.go index f79bca6d3..0c0d12a3e 100644 --- a/internal/web/service/inbound_traffic.go +++ b/internal/web/service/inbound_traffic.go @@ -21,6 +21,10 @@ import ( "gorm.io/gorm/clause" ) +// A client with a renewal day set auto-renews too, so it must not read as +// depleted — otherwise the operator's purge deletes it between cycles (#6239). +const depletedClientsClause = "reset = 0 and reset_day = 0 and ((total > 0 and up + down >= total) or (expiry_time > 0 and expiry_time <= ?))" + func (s *InboundService) AddTraffic(inboundTraffics []*xray.Traffic, clientTraffics []*xray.ClientTraffic) (needRestart bool, clientsDisabled bool, err error) { var disabledNodeIDs []int err = submitTrafficWrite(func() error { @@ -835,7 +839,7 @@ func (s *InboundService) DelDepletedClients(id int) (err error) { // Collect depleted emails globally — a shared-email row owned by one // inbound depletes every sibling that lists the email. now := time.Now().Unix() * 1000 - depletedClause := "reset = 0 and ((total > 0 and up + down >= total) or (expiry_time > 0 and expiry_time <= ?))" + depletedClause := depletedClientsClause var depletedRows []xray.ClientTraffic if err := tx.Model(xray.ClientTraffic{}). Where(depletedClause, now). diff --git a/internal/web/service/server.go b/internal/web/service/server.go index 41e1479d6..64883705c 100644 --- a/internal/web/service/server.go +++ b/internal/web/service/server.go @@ -1488,25 +1488,37 @@ func restoreHostBoundSettings(snap hostBoundSnapshot) { if db == nil { return } + settingSvc := &SettingService{} for _, key := range hostBoundSettingKeys { if _, had := snap.present[key]; !had { - // No row here before the import, so the default applied. Drop the - // imported row rather than inherit the source machine's value. + // Absent because it is minted on demand, not because a default applied: + // the imported copy is the only one that exists, so keep it (#6227). + if lazilyMintedSettingKeys[key] { + continue + } if err := db.Where("key = ?", key).Delete(&model.Setting{}).Error; err != nil { logger.Warningf("Import: could not drop imported setting %q: %v", key, err) } continue } - // The imported row may or may not exist; settings are key-value, so an - // upsert keyed on the name is the only safe write here. - if err := db.Where(model.Setting{Key: key}). - Assign(model.Setting{Value: snap.values[key]}). - FirstOrCreate(&model.Setting{}).Error; err != nil { + // saveSetting rather than Assign(struct): GORM drops zero-valued fields from + // the assignment map, so an empty local value never overwrote the import. + if err := settingSvc.saveSetting(key, snap.values[key]); err != nil { logger.Warningf("Import: could not restore setting %q for this machine: %v", key, err) } } } +// Minted on demand, so a fresh install has no row: dropping the imported copy +// would destroy the only one that exists, CA private key included. +var lazilyMintedSettingKeys = map[string]bool{ + "nodeMtlsCaCertPem": true, + "nodeMtlsCaKeyPem": true, + "nodeMtlsClientCertPem": true, + "nodeMtlsClientKeyPem": true, + "nodeMtlsClientCAPem": true, +} + func (s *ServerService) ImportDB(file multipart.File, keepHostSettings bool) error { if database.IsPostgres() { return s.importPostgresDB(file, keepHostSettings)