fix: follow-ups from the post-merge reviews of #6221, #6227, #6230 and #6239 (#6250)

* 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.
This commit is contained in:
n0ctal
2026-08-18 18:24:59 +05:00
committed by GitHub
parent abd320994a
commit 3f1dd4bf5a
11 changed files with 169 additions and 27 deletions
+11 -8
View File
@@ -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
}
@@ -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")
}
}