* fix(nodes): say which half of node mTLS failed, and say it as an error
A configured client CA bundle that will not parse produced the same
warning as a settings read that failed, and both read as though mTLS
were merely unavailable. It is not: the node API silently stops
accepting client certificates, callers fall back to a bearer token or
lose their only credential, and the one line saying so is a warning at
boot.
Report it at error level, and distinguish the two causes rather than
attributing a storage fault to the operator's certificate bundle.
NodeMtlsClientCAPool now tags the parse failure with
ErrNodeMtlsTrustBundleInvalid; its message text is unchanged, so
anything matching on the existing string still matches.
Startup is deliberately left alone. Refusing to boot was considered and
rejected: the bundle is one of two equal credentials here, a panel that
will not start takes the proxies and the subscription server with it,
and bundles written before the stricter validation landed in #6188 are
already stored, editable only through the panel that would no longer
come up.
The tests pin the tag on an unusable bundle and its absence on an unset
one; without the tag the first goes red.
* test(nodes): drop a duplicate node mTLS trust-bundle test
TestNodeMtlsClientCAPoolLeavesUnsetBundleUntagged asserted only that an
unset nodeMtlsClientCAPem yields (nil, nil). That path returns before the
line the sentinel change touched, so the test was green with and without
ErrNodeMtlsTrustBundleInvalid, and TestNodeMtlsClientCAPool already pins
the same two assertions on the same fixture. A test that passes either way
certifies nothing and then gets cited as coverage for the sentinel.
TestNodeMtlsClientCAPoolTagsAnInvalidBundle, which does go red without the
sentinel, stays as the regression guard.
---------
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
autoRenewClients collects every expired client that carries a reset interval,
but three of them never reach a new window: one may be missing from its
inbound's settings, one may resolve to no whole interval, and one may still
land in the past once the reset cap truncates the catch-up. All three keep
their counters and their expiry on purpose.
clearGlobalTraffic was still called with the full candidate list, so those
three lost their cross-panel rows while their local counters stayed. The next
push recreates the rows, and the expiry branch of the depletion check cuts
these clients regardless, so nothing is served past its limit — but between the
delete and the next push the cross-panel view under-reports them, and since the
expiry never advances that repeats on every poll.
Pass only the clients whose counters this pass reset. clearGlobalTraffic
already early-returns on an empty list, so a poll that renews nobody stays a
no-op rather than deleting every row. The renewed count returned to the caller
now counts the same set, instead of reporting candidates as renewals.
Tests cover both directions: a capped catch-up keeps its rows, and an actually
renewed client still loses them, since stale pushed totals would otherwise
re-deplete the fresh window at once.
* 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.
The tracked frontend/public/mockServiceWorker.js is generated by msw and pinned
at 2.14.7, while package-lock.json installs 2.15.0. msw rewrites the worker on
postinstall, so npm ci leaves the tree dirty on a clean checkout and every
contributor either commits an unrelated 30-line diff or discards it.
Regenerate the worker for the locked version and add a check that compares it
against the installed runtime, wired into make verify and CI so the pair cannot
drift again.
* feat(clients): give each client its own traffic reset cycle
Traffic reset is configured on the inbound, so every client sharing an
inbound resets together. An operator running a monthly 1000GB plan and a
weekly 200GB plan side by side has to press Reset Traffic by hand.
Clients now carry the same trafficReset / trafficResetDay pair the
inbound already has, with the same vocabulary and the same monthly
due-day rule, and PeriodicTrafficResetJob makes a second pass over the
clients whose own cycle matches the period it is running for. A client
that leaves the field at never behaves exactly as before: only its
inbound's schedule can reset it.
The fields live on ClientRecord as well as in the inbound settings JSON,
so an ordinary edit does not write the cycle back as empty, and an
unknown period is rejected rather than coerced, since a coerced value
would read as configured while no job would ever select the client.
Cron expressions and a custom post-reset quota from the issue are left
out: both are separate decisions, and neither has an inbound-level
counterpart to stay consistent with.
* fix(clients): make the per-client reset cycle editable and safe to run
Review found three things wrong with the first cut, one of them mine and
worse than the bug it replaced.
The cycle could only be set at creation. ClientService.Update writes the
columns directly only for a client with no inbounds; the normal path goes
through SyncInbound and applyClientRecordMerge, which this change had not
extended, so an edit updated the settings JSON while the clients column
kept the old value and the job kept applying the old cycle. The earlier
test passed because it asserted the value survived an unrelated edit,
which it did precisely because nothing ever wrote it. Replaced with a
test that changes the cycle and switches it off again.
Avoiding the re-enable that ResetTrafficByEmail performs was wrong.
Depletion disables clients.enable and the settings JSON as well as
client_traffics.enable, so lifting only the quota gate left a depleted
client out of the generated config with zeroed counters, which no longer
match the depleted predicate: locked out permanently. The rule is now
about cause, not state — a client the quota switched off is restored, one
disabled below its quota was switched off by hand and is skipped.
The bulk path also bypassed node propagation and the MTProto sidecar
quota that ResetTrafficByEmail handles, so it silently did nothing on
node-backed inbounds. Dropped in favour of the integrated path, whose
needRestart is now collected and turned into a single SetToNeedRestart.
Also adds the AutoMigrate NULL backfill, guards the merge so a stale node
snapshot cannot erase a configured cycle, validates the bulk-create and
import paths, normalizes the day the way the inbound path does, marks the
fields omitempty so existing clients match the published contract, and
shares one TRAFFIC_RESETS tuple between the three forms.
* fix(clients): validate renew fields on the bulk and import paths too
BulkCreate and ImportClients insert client records without going through
Create, so the resetDay/resetMax checks added with the calendar renewal
(#6239) and the renew cap (#6238) never ran there. An API caller could
store resetDay 45 or a negative resetMax, values the renewal query then
mishandles silently. Mirror Create's validation on both batch paths, next
to the trafficReset check they already carry.
---------
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
* feat(clients): renew on a calendar day instead of a rolling interval
Auto-renew advances the expiry by a fixed number of milliseconds, so a client
set to 30 days drifts against the calendar: renewing on 31 January lands on
2 March, and by the end of the year the billing day has wandered a fortnight
from where the operator's own plan resets.
Add a per-client renewal day. When set, the expiry steps whole calendar months
at midnight in the panel's time zone. A month too short for the chosen day
renews on its last day and the following month returns to the chosen one, so
the 31st does not decay into the 28th permanently.
Zero keeps the interval mode, so existing clients are untouched.
The interval branch now also refuses a zero step. It is unreachable while the
selection filter holds, but that loop runs on the single traffic writer, and a
zero interval there would hang every panel mutation behind it.
* fix(clients): persist the calendar renewal day on the client record
resetDay lived only in the inbound settings JSON and client_traffics, so
every path that rebuilds a client from the clients table wrote it back as
zero: an ordinary edit, an attach to a second inbound, a traffic reset on
a disabled client. Calendar mode turned itself off during normal use and
the operator only found out a month later.
Adds reset_day to ClientRecord and threads it through ToRecord, ToClient,
applyClientRecordMerge and the record update map, so the value survives
the round trip. The clients page filter and ClientSlim now recognise the
mode, nodeClientRenewed classifies a calendar renewal as a renewal, the
node snapshot merge carries reset_day, and the service layer rejects a
day outside 0-31 rather than clamping it silently.
Also renames the label keys to renewOnDay to keep them apart from the
existing renewDays, translates them and the new RESET_DAY subscription
placeholder in all 13 locales, adds the field to the bulk-add modal, and
drops the stray internal/web/dist/.gitkeep build stub.
* fix(clients): let the billing day be changed after creation
ClientService.Update writes the record columns directly only for a client
with no inbounds. The normal path goes through SyncInbound and
applyClientRecordMerge, which this change had not extended, so moving a
client from the 20th to the 5th updated the inbound settings JSON while
clients.reset_day kept the old value and the renewal kept using it.
The existing test did not catch it: it asserted the day survived an
unrelated edit, and it survived precisely because nothing on that path
ever wrote it. TestClientEditChangesTheBillingDay moves the day and then
switches calendar mode off again; removing the record write turns it red.
* 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.
---------
Co-authored-by: n0ctal <n0ctal@users.noreply.github.com>
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
* 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>
* feat(server): keep this machine's own settings when importing a database
Import replaces the database wholesale, so the uploaded file's listen
addresses, ports, base path, certificate paths and node identity land on the
destination. Moving a configuration to a new host therefore leaves the panel
answering on an address it does not own, presenting certificates it does not
have, and claiming the source machine's identity towards its nodes.
Capture the host-bound settings before the swap and write them back once the
imported database opens. Everything else — inbounds, clients, templates, the
rest of the settings — still comes from the file.
A checkbox controls it, defaulting to keeping this machine's values; clearing
it restores the old behaviour for anyone deliberately cloning a host.
* fix(server): drop imported host settings this machine never had, and cover Postgres
Two gaps in the previous commit. The snapshot only recorded rows that existed,
so a key with no row here — the default for every certificate path, both listen
addresses and all the node mTLS material — kept the imported value: exactly the
case the change is meant to fix. The snapshot now records which keys were
absent and deletes the imported row for them, letting the default apply again.
The PostgreSQL path took the flag and ignored it, so a dump restore still
adopted the source machine's settings. It now captures and restores the same
way the SQLite path does.
* 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.
---------
Co-authored-by: n0ctal <n0ctal@users.noreply.github.com>
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
* feat(clients): cap how many times a client may auto-renew
Auto-renew today runs forever: a prepaid or fixed-term client keeps being
handed new periods until an operator remembers to switch it off. There is no
way to say "renew this three times, then let it lapse".
Add a per-client maximum. Zero keeps today's behaviour, so nothing changes for
anyone who does not set one. When the count is reached the client is simply
left to expire, like any client without auto-renew.
Catching up several missed periods spends one allowance per period. A client
that was away for three cycles must not receive three of them free of the cap,
and the catch-up stops at the last period the cap paid for rather than jumping
to the present.
* fix(clients): persist the auto-renew cap and stop the capped churn
resetMax lived only in the inbound settings JSON and client_traffics, so
every path that rebuilds a client from the clients table wrote it back as
zero. The edit dialog showed 0 for a capped client, and saving an
unrelated comment change lifted the cap; an attach or a traffic reset did
the same with no operator action at all.
Adds reset_max to ClientRecord and threads it through ToRecord, ToClient,
applyClientRecordMerge, the record update map and ClientSlim, so the cap
survives the round trip.
When the cap truncates a catch-up the client is still expired, but the
renewal side effects fired anyway: counters were zeroed for periods it
can never use, and it was enabled and pushed to xray only for
disableInvalidClients to undo both in the same transaction. Those are now
skipped when the new expiry has not reached the present.
Also makes any non-positive resetMax mean unlimited instead of silently
meaning "never renew again", rejects a negative one at the service layer,
surfaces renewals used against allowed in the client info modal so the
operator can see what to raise, adds the field to the bulk-add modal,
translates the labels in all 13 locales, and drops the stray
internal/web/dist/.gitkeep build stub.
* fix(clients): let the renewal cap be changed after creation
ClientService.Update writes the record columns directly only for a client
with no inbounds. The normal path goes through SyncInbound and
applyClientRecordMerge, which this change had not extended, so raising a
cap from 3 to 6 — the natural action when a customer buys another block
of periods — updated the inbound settings JSON while clients.reset_max
kept the old value and the renewal query kept enforcing it.
The existing test did not catch it: it asserted the cap survived an
unrelated edit, and it survived precisely because nothing on that path
ever wrote it. TestClientEditChangesTheRenewalCap raises the cap and then
lifts it entirely; removing the record write turns it red.
* 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.
---------
Co-authored-by: n0ctal <n0ctal@users.noreply.github.com>
* fix(node): keep disabled inbounds the node snapshot cannot report
A node builds its traffic snapshot from the inbounds Xray is actually running,
so an inbound with enable=false is never in it. The central sweep reads that
absence as "the node no longer has this inbound" and deletes the row, its
clients' traffic history and its port reservation — on a perfectly healthy
node, with no way to tell it apart from a real deletion.
Disabling an inbound in the panel and waiting one sync interval is enough to
lose it. Skip disabled inbounds in the sweep: their absence carries no
information, and an explicit delete still removes them.
* 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.
* fix(inbounds): close the port check-and-claim race on the serial writer
AddInbound reads the port conflict outside its transaction and then commits in
a bare db.Transaction, so two overlapping creates both pass the read and both
insert. UpdateInbound already runs on the single traffic writer, and so does
the node snapshot path; AddInbound is the one inbound writer left out.
Move it onto runSerializedTx and evaluate the conflict inside the transaction,
in both AddInbound and UpdateInbound. The check and the claim then commit
together on one goroutine, which closes the window on SQLite (immediate write
lock) and PostgreSQL alike without new schema, locks or configuration.
The wildcard/specific pair is the case worth naming: those are two distinct
rows, so no unique index can reject them — only the semantic check can, and
only if nothing can interleave between it and the insert.
* fix(inbounds): restore the port check UpdateInbound lost
The previous commit deleted UpdateInbound's pre-flight conflict check and never
added the in-transaction one, so editing an inbound onto an occupied port was
accepted outright. No test covered that path, so CI stayed green.
Evaluate the conflict inside the transaction, as AddInbound already does, and
add the regression test that fails without it.
* 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.
---------
Co-authored-by: n0ctal <n0ctal@users.noreply.github.com>
* feat(i18n): translate the log levels, access events and calendar labels
The log-level selector, the access-log event tags, the Sub Formats sidebar
entry and the calendar choices were hardcoded English, so a fully translated
locale still showed them in English on core screens.
Add eleven keys across the 13 locales and reference them. Russian and
Ukrainian are translated; the remaining locales carry the English string, the
same convention the existing files already use for untranslated entries.
Two module-level constants had to move: the calendar list and the access-event
map were built outside the component, where t is not in scope. The event map
now stores keys and resolves them at render.
* fix(i18n): keep the log export language-independent and fit the translations
Three follow-ups from review. The downloaded x-ui.log had started carrying the
translated event text, so its contents depended on the panel language and the
Russian value for PROXY contains a space in a field format whose other values
are single tokens. The export keeps DIRECT/BLOCKED/PROXY; only the on-screen
tag is translated.
The log-level select had a fixed 95px width sized for "Warning", which clips
"Предупреждение"; it now grows with its content.
The three access filters stayed English while the tags they filter became
translated, so they use the same keys.
---------
Co-authored-by: n0ctal <n0ctal@users.noreply.github.com>
* ci: actually run the PostgreSQL schema and migration tests
TestHostAutoMigrateCreatesColumns_Postgres and TestMigrate_Postgres skip
unless XUI_DB_TYPE and XUI_DB_DSN are set. CI sets them only for the
durable-first step, so both tests have never run: a green pipeline says
nothing about the PostgreSQL schema or the migration path.
The job already has a PostgreSQL service. Point those two tests at it and
fail if either skips, the same guard the durable-first step uses.
* ci: make the PostgreSQL guard fail on a renamed test, and self-test the workflow
The guard asserted the absence of `--- SKIP`, which only catches a test that
ran and skipped. A renamed or deleted test makes `-run` match nothing, so
`go test` prints "no tests to run" and exits 0 — the step stays green while
testing nothing, which is the exact failure this PR set out to close.
Both steps now count `--- PASS` lines and require the expected number: at
least one for durable-first, exactly two for the schema tests.
Also adds `.github/workflows/ci.yml` to both `paths` filters so a change to
the workflow runs the workflow — without it this PR's own CI never fired and
the new step would first execute on main after merge — and hoists the
duplicated DSN to job-level `env`.
---------
Co-authored-by: n0ctal <n0ctal@users.noreply.github.com>
* ci(release): stamp released binaries with their source revision
The release job builds `main.go`, which Go records as a file list rather than a
package, so the binary carries no VCS metadata at all: `go version -m xui-release`
reports command-line-arguments and nothing else. There is no way to tell which
commit a published binary came from, which is exactly what you want when a user
reports a bug against "the latest release".
Build the package with -buildvcs=true instead. Same output, same flags, plus
vcs.revision and vcs.time in the binary.
* ci(release): give the Windows job the git it needs to stamp
-buildvcs=true fails the build when it cannot read VCS state, and the MSYS2
shell the Windows job runs in has no git on PATH. Install it there so the
Windows binary carries the same revision as the Linux ones instead of the flag
turning into a build break.
---------
Co-authored-by: n0ctal <n0ctal@users.noreply.github.com>
The orphan sweep deletes a central inbound and the traffic history of every
client on it, but wrote nothing. An inbound that vanishes minutes after being
created is then indistinguishable from one that never arrived, and the only
way to tell them apart is reading the source.
Name the node, tag, id and port so the removal is visible in the panel log.
Co-authored-by: n0ctal <293235942+n0ctal@users.noreply.github.com>
* feat(inbounds): add a narrow endpoint for subscription sort order
Changing an inbound's position in subscription output currently goes through
/update/:id, which takes a whole inbound: the caller has to send settings and
the entire client list back, and whatever it read before the edit is what gets
written. Two people reordering and editing clients in the same inbound race on
one blob, and the reorder wins by overwriting.
Mirror the existing /setEnable/:id shape. The handler takes only the index and
the service reads the stored inbound, so nothing in the request can reach the
settings JSON. Node-owned inbounds are marked dirty in the same transaction and
pushed through the existing runtime update.
* fix(nodes): scope sub sort index updates
---------
Co-authored-by: n0ctal <293235942+n0ctal@users.noreply.github.com>
* fix(nodes): validate every certificate in the node mTLS trust bundle
AppendCertsFromPEM reports success once a single certificate parses, so a
trust bundle whose later entries are damaged or truncated was accepted with
those entries silently absent from the pool. Parse and validate every PEM
block instead, and reject the bundle if any of them is malformed.
* fix(mtls): reject malformed certificate bundle layout
---------
Co-authored-by: n0ctal <293235942+n0ctal@users.noreply.github.com>
* fix(sub): show copy-only page for browser subscription visits
Browser navigation to /sub previously rendered the normal subscription page, which exposed subscription material in page data or raw base64 depending on request headers. Keep VPN clients on the raw subscription body, but classify browser document requests and return a neutral static copy-only HTML page with no embedded share links or page data.
This preserves the C1 LimitIP parser fix in the same master candidate while avoiding a DE rollback of the browser subscription UX.
* fix(sub): keep the themed page for an explicit html request
Only implicit browser navigation is downgraded to the copy-only page. An
operator who appends html=1 or view=html already holds the URL, so the
themed subscription page keeps rendering for them and serveSubPage stays
in use.
* fix(sub): keep browser pages copy-only
* feat(sub): warn when salamander settings cannot reach the client
A hysteria2 share link carries obfuscation as obfs=salamander plus
obfs-password, and nothing else. Xray's finalmask accepts more than that —
packetSize among them — and those extra settings change what the server expects
on the wire. The emitted URI then looks complete but describes a server the
client cannot reach: every standard client applies plain salamander, the server
drops the packets, and the failure is silent on both ends.
Log the unexpressible keys when building such a link, naming the inbound, so the
cause is visible instead of appearing as a client-side problem.
* fix(sub): deduplicate salamander warnings
* fix(sub): use a fullwidth percent in USAGE_PERCENTAGE
A remark is placed in the share link fragment, so an ASCII percent is
percent-encoded to %25. Happ treats such a fragment as malformed, discards the
whole remark and falls back to showing the server hostname, which defeats the
point of a remark template and leaks the host into the client's server list.
Emit U+FF05 FULLWIDTH PERCENT SIGN instead. It renders the same to a reader,
never produces %25, and round-trips through url.Parse unchanged.
* test(sub): exercise production fragment encoding
---------
Co-authored-by: n0ctal <293235942+n0ctal@users.noreply.github.com>
* fix(cli): stop -getApiToken accumulating admin tokens
`x-ui setting -getApiToken` reads like a getter, but when tokens already exist
it minted a brand-new one named `cli-fallback-<unix>` on every invocation. The
plaintext is printed once and the row stays enabled forever, so an operator who
runs the command a few times while debugging silently leaves several
admin-equivalent credentials behind that nobody can tell apart or revoke
knowingly.
Keep the convenience the fallback was added for, but rotate a single
`cli-fallback` token instead: RecreateByName drops any existing row with that
name before issuing a new one, so at most one CLI-issued token exists at a time
and the previous plaintext stops working.
* fix(api-token): preserve token on failed replacement
---------
Co-authored-by: n0ctal <293235942+n0ctal@users.noreply.github.com>
* fix(database): keep IP limits when the fail2ban probe is inconclusive
ResetIpLimitNoFail2ban clears limitIp on every client — inbound settings JSON
and the clients table — whenever fail2banCanEnforce() returns false, then
records itself in the seeder history so it never re-evaluates. The probe was a
single `fail2ban-client -h` run, so it answered false both when fail2ban is
genuinely absent and when the command merely failed that once: a panel that
starts before fail2ban is up, or in a container where it is installed a moment
later, permanently loses every configured limit with no log line and no way
back.
Separate the two. A missing binary still means "absent" and the cleanup runs as
before; a binary that exists but will not run is reported as unknown, leaves the
configured values untouched, logs why, and does not record the seeder, so the
next start decides again.
* test(database): cover fail2ban reset safeguards
---------
Co-authored-by: n0ctal <293235942+n0ctal@users.noreply.github.com>
* fix(web): report unexpected HTTP serve failures
* test(web): cover normal close and all HTTP servers
---------
Co-authored-by: n0ctal <293235942+n0ctal@users.noreply.github.com>
npm audit --omit=dev --audit-level=high is a CI gate and it currently fails on
main: swagger-ui-react pulls @swagger-api/apidom-reference, which pins
minimatch, which resolves brace-expansion to 5.0.8 — the range covered by
GHSA-rgw5-rvv9-x895.
Pin the patched 5.0.9 through the existing swagger-ui-react overrides block
rather than globally: minimatch@3 under eslint-plugin-jsx-a11y still needs the
1.x line, and a blanket override would force v5 there too.
* fix(nodes): make node API tokens write-only
* fix(nodes): keep token optional on edit for write-only API tokens
NodeView no longer returns apiToken, so the edit form must consume hasApiToken and not require re-entering the token. Relaxes the form validation on edit, adds a keep-current placeholder, and adds the i18n key to all 13 locales.
* fix(clients): surface bulk-reset auto-enable failures
BulkResetTraffic re-enables a disabled client before resetting its
traffic, but discarded the s.Update result with `_, _ =`, so a failed
re-enable was silent: the client stayed disabled with nothing logged,
unlike the single-client ResetTraffic path which already warns on the
same call. Check the error and log a warning to match, and add a
regression test covering BulkResetTraffic's previously-untested
re-enable path.
* ci: update Go toolchain for govulncheck
---------
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
* fix(clients): parse only settings.clients across protocols
Several inbound settings readers decoded the whole settings object into map[string][]model.Client. Real protocol settings include scalar keys such as VLESS decryption and Hysteria version, so that shape can fail before callers reach settings.clients or leave them relying on decoder side effects.
Add one shared helper that extracts only the clients field through json.RawMessage, then use it from GetClients, SearchClientTraffic and the IP-limit job fallback paths. Regression tests cover VLESS and Hysteria settings with scalar protocol fields.
* fix(clients): reject empty inbound settings
* fix(sync): mark node dirty inside the mutation transaction
ConfigDirty is currently set by MarkNodeDirty AFTER the mutation, on a
separate DB handle outside the mutation's transaction. A crash or error
between the committed change and the mark leaves a committed config
change that never reconciles to the node (silent drift). Add
MarkNodeDirtyTx(tx, id) and call it inside each mutation's transaction so
the dirty mark commits atomically with the change.
* fix(test): initialize DB in TestResolveInboundAddress and group gorm import
Two CI failures on this branch:
- race (-shuffle=on): TestResolveInboundAddress reaches resolveInboundAddress -> configuredPublicHost -> GetSubDomain, which reads the global DB. The test never initialized one, relying on another sub-package test to do so first; under shuffle it ran first and nil-dereferenced gorm. Call initSubDB(t) so it is self-sufficient (empty DB yields an empty subDomain, so the subscriber-host fallback still holds).
- golangci goimports: gorm.io/gorm was grouped with the github.com/mhsanaei/3x-ui local imports in node_dirty_test.go. Move it into the third-party group.
* fix(settings): require server-side 2fa for sensitive changes
* fix(lint): group third-party imports separately from local (goimports)
golangci-lint goimports flagged setting.go and setting_security_test.go because xlzd/gotp and gorm.io/gorm were mixed into the github.com/mhsanaei/3x-ui local-prefix group. Move them into the third-party group so the local imports stand alone.
The external subscription fetcher read the remote body with a plain
io.LimitReader, silently truncating at 2 MiB and decoding whatever
prefix arrived (possibly a half share link). Detect the overflow with
the established N+1 pattern and return an error so the caller serves the
last cached value instead of a corrupted partial list.
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
* perf(xray): compile log/traffic regexps once at package scope
GetTraffic recompiled two stats regexps on every traffic tick, and LogWriter.Write
recompiled two more on every log line. Hoist all four to package-level vars so they
compile once at load instead of per call on hot paths.
* fix(xray): guard LogWriter.lastLine against the GetResult reader race
Write is driven by the Xray process goroutine while Process.GetResult
reads lastLine from the caller's goroutine, so the unsynchronized field
is a data race under `go test -race`. Add an RWMutex and route every
write through setLastLine; GetResult reads via LastLine().
* fix(xray): bound handler gRPC calls with a deadline
AddInbound, DelInbound and the AddUser AlterInbound call used
context.Background(), so a hung core connection could block the caller
indefinitely (for example while the process restart lock is held). Give
them a 10s deadline (handlerRPCTimeout) and a nil-client guard, matching
the other handler operations.
getSetting (WHERE key=?) runs on nearly every subscription request and job
tick and had no index, so each lookup full-scans the settings table past the
large xrayTemplateConfig blob. Add an index on settings.key; AutoMigrate
creates it on existing DBs too. Includes a HasIndex test.
GetTraffic recompiled two stats regexps on every traffic tick, and LogWriter.Write
recompiled two more on every log line. Hoist all four to package-level vars so they
compile once at load instead of per call on hot paths.
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
NodeService.Delete dropped the node row (and its per-node child rows) without
checking for inbounds still referencing it via node_id, leaving orphaned
inbounds with a dangling node_id that confuse node sync, subscriptions and
cleanup. Refuse the delete with a clear error when inbounds are still attached,
and remove the per-node child rows before the node row inside one transaction.
Delete stays tolerant of a missing node row so it can still clean up orphaned
rows. Regression test covers the blocked and clean-delete paths.
The public subscription http.Server set no timeouts, leaving the most exposed
listener open to slow-header/Slowloris exhaustion. Mirror the panel server
timeouts already used in internal/web/web.go.
Remote node HTTP responses were read with an unbounded io.ReadAll, so a
broken or hostile node could force the master panel to buffer an arbitrarily
large body. The single Remote.do choke point that all node calls funnel
through now:
- validates the HTTP status before reading any success payload (a non-OK
body is only read up to a small bounded diagnostic snippet, so a node
cannot make the master buffer a large body just to return an error);
- fast-fails on an honestly-declared oversize Content-Length;
- reads the success body through readCappedBody, an io.LimitReader cap
(64 MiB) that rejects oversize with a typed error.
The 64 MiB cap bounds one response's wire/decompressed size; it is documented
as not a process-wide memory bound (endpoint-specific caps and a concurrency
budget remain follow-ups).
Tests cover the cap+1 boundary, an oversize streamed body, a normal envelope,
and non-OK status precedence.
A panic in a goroutine without a recover takes the whole panel down. The
per-node heartbeat and traffic-sync goroutines run remote network I/O for
each node with no panic isolation, so one misbehaving node could crash the
master.
Add common.GoRecover(name, fn), which runs fn in a goroutine guarded by a
recover that logs the panic with a stack trace instead of crashing, and use
it for the per-node heartbeat, traffic-sync and global-push goroutines. The
deferred WaitGroup/semaphore releases still run during panic unwind, so the
group never stalls. Other background goroutines can adopt the same helper.
The scheduler was created without a panic recovery wrapper, so a panic in any
scheduled job (traffic write, IP check, etc.) propagated up and could take down
the whole panel process. Wrap jobs with cron.Recover so a panic is logged and
the scheduler keeps running.
* fix(xray): verify the release archive checksum before installing
UpdateXray downloaded the Xray-core release zip and installed the binary
from it after only a TLS fetch, an HTTP-200 check and a size cap — the
archive itself was never verified, so a corrupted or tampered release
asset would be extracted and run as the panel's xray binary.
Verify the downloaded archive against the SHA2-256 published in the
release's .dgst sidecar (which XTLS ships next to every asset) before
installing, and abort the update on mismatch, a missing/short SHA2-256
entry, or an unreachable .dgst. The digest parser and fetch are covered by
tests, including the real .dgst line format ("SHA2-256= <hex>").
* address review: clearer warning + re-download guidance on checksum mismatch
Per review feedback on the PR: on a SHA-256 mismatch, surface a plain-language
warning that the downloaded archive is corrupted or differs from the official
release and that the user should exit and re-download, instead of a terse
"checksum mismatch" error. The install still aborts so a mismatched binary is
never run; the message now tells the user the safe next step.
The process cmd, done and exitErr fields were written by Start/startCommand and
the waitForCommand goroutine while IsRunning/GetErr/GetResult/Stop read them
concurrently from other goroutines (the status endpoint and the check-xray
job) — a data race. Guard them with a RWMutex: writers take the write lock;
readers snapshot under the read lock and run any blocking syscall
(Wait/Signal/Kill) on the local copy without holding it. IsRunning now uses the
done channel as the exit signal instead of reading cmd.ProcessState, which
races with cmd.Wait. Adds a -race regression test.
* fix(subscriptions): avoid shared mutable state during generation
* fix(subscriptions): serve external-link-only subs in JSON/Clash; load remark settings per request
The ForRequest refactor added an early `len(inbounds) == 0` return to
GetJson/GetClash that fired before external links were fetched, so a
subscription whose only entries are external links (or whose inbounds are
all disabled) rendered empty in the JSON and Clash formats. Drop the
premature check — the existing inbounds+externalLinks empty guard already
covers the truly-empty case.
Also load datepicker/emailInRemark in PrepareForRequest rather than only in
getSubs, so JSON and Clash remarks honor these settings instead of seeing
the zero values (emailInRemark previously depended on the shared-state leak
this PR fixes).
Add a regression test covering an external-link-only sub across both formats.
---------
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
* feat(web): cap request body size on state-changing routes
* fix(web): exempt importDB from request body size cap
The 10 MiB body cap was applied globally, which would break database
restore (/panel/api/server/importDB) on any panel whose SQLite backup
exceeds the limit. Make MaxBodyBytes accept exempt path suffixes and
pass importDB through uncapped; the cap still covers all other
state-changing routes. Add a test for the skip-suffix behavior.
---------
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
