Feature/teminuosi inbound tools - #6335
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const secrets: Parameters<typeof applyPresetSecrets>[1] = { | ||
| subId: await fetchCommonSubId(), |
There was a problem hiding this comment.
🔴 Every preset-created client is stamped with one panel-wide subId, so unrelated customers end up sharing a subscription URL and can read each other's credentials.
applyPresetSecrets overwrites subId on every client in the preset row (inbound-presets.ts#L247-L256), and applyPreset is auto-invoked on every Add-Inbound modal open, not only when a preset card is clicked:
3x-ui/frontend/src/pages/inbounds/form/InboundFormModal.tsx
Lines 619 to 623 in 497a257
In 3x-ui subId is the subscription identity: getInboundsBySubId selects every enabled inbound joined to any client with that sub_id, and matchingClients returns all of them per inbound:
Lines 481 to 494 in 497a257
Scenario: admin opens Add Inbound (preset auto-applies), saves, hands customer A https://panel:2096/sub/<COMMON>. Weeks later the same flow creates an inbound for customer B — B's link is the identical string. Fetching it returns A's full VLESS UUID / Trojan password, and the sub page lists A's email and traffic. Before this PR the default was a per-client random id (subId: seed.subId ?? RandomUtil.randomLowerAndNum(16), inbound-defaults.ts#L60-L62). This is on the public internal/sub/ surface.
The PR's own test pins the behaviour (applyPresetSecrets stamps a shared subId on every client, inbound-presets.test.ts#L74-L80), so it needs a design change rather than a one-line patch: the "one URL for all my nodes" goal only holds within a single subscriber. Generate a fresh random id per inbound and reuse a batch-local id only inside addAllRecommended; drop the persisted panel-wide commonSubId setting and its endpoint entirely.
(Secondary, same design: SettingService.GetCommonSubId is a lock-free read-modify-write on a table whose key index is not unique — model.go#L768-L770 — so two concurrent calls insert two commonSubId rows and the second caller's id is orphaned. Removing the setting removes this too.)
| g.GET("/:id/orphanCount", a.getOrphanCount) | ||
| g.POST("/orphanCountBatch", a.getOrphanCountBatch) |
There was a problem hiding this comment.
🔴 Three new /panel/api/* routes with no entry in the endpoint registry — TestRouteRegistryContract fails, so make test-go and the go-test CI job are red.
New routes in this PR:
GET /panel/api/inbounds/:id/orphanCount(this line)POST /panel/api/inbounds/orphanCountBatch(next line)GET /panel/api/server/getCommonSubId(server.go#L58-L60)
frontend/src/pages/api-docs/endpoints.ts is not in the changed-file list, and grepping the head for orphanCount|getCommonSubId never hits it. The contract test builds the real router and diffs it against the registry both ways for everything under /panel/api/:
3x-ui/internal/web/routes_contract_test.go
Lines 38 to 41 in 497a257
Per CLAUDE.md: "New g.POST/g.GET in internal/web/controller/ REQUIRES a matching entry in frontend/src/pages/api-docs/endpoints.ts, then make gen", plus the fourth step nothing checks: "copy frontend/public/openapi.json → docs/public/openapi.json, then cd docs && pnpm gen:api". None of frontend/src/generated/, frontend/public/openapi.json or docs/public/openapi.json were regenerated, so make verify's gen-check fails too.
(If the commonSubId endpoint is dropped per the other review comment, only the two orphanCount routes need registering.)
| if (cert?.certFile && cert.keyFile) { | ||
| secrets.certFile = cert.certFile; | ||
| secrets.keyFile = cert.keyFile; | ||
| secrets.domain = domain; | ||
| } | ||
| applyPresetSecrets(row, secrets); |
There was a problem hiding this comment.
🔴 A TLS preset on a panel with no web certificate silently saves an inbound with security: "tls" and a certificate entry holding no certificate at all.
The guard has no else and does not abort, while the sibling batch path does skip cert-needing presets:
3x-ui/frontend/src/pages/inbounds/form/InboundFormModal.tsx
Lines 428 to 432 in 497a257
With no web cert, fetchTargetCertificate() returns {certFile:'', keyFile:'', domain:''}, so emptyFileCert() (inbound-presets.ts#L86-L98) stays blank. It still passes the submit gate: TlsCertSchema is a union and the inline arm only requires certificate/key to be arrays, which [] satisfies (tls.ts#L39-L55). So certificates:[{certificate:[],key:[],…}] is POSTed, and nothing on the Go side rejects it — AddInbound validates finalmask/reality/mtproto/amneziawg only.
In simpleMode the stream and security tabs are not rendered, so there is no cert field anywhere in the one-click flow to notice or fix it. Affects trojan-tls, vmess-ws-tls and hysteria2.
This repo already states the consequence:
3x-ui/internal/web/runtime/remote.go
Lines 832 to 841 in 497a257
I could not read xray-core here to name the deciding upstream symbol, so the exact failure mode is unverified — but per the comment above it takes the whole Xray process down, not just this inbound. Reuse the hasCertificate check addAllRecommended already computes: refuse the apply (or surface an error) when preset.needsDomain and the panel has no cert.
| // migrateRemarkTemplateDefault moves panels that started on the v3.4.0 default | ||
| // subscription template to the v3.4.1 default that includes the client email. | ||
| // Exact-match only: operator-customized templates are preserved. | ||
| func migrateRemarkTemplateDefault() error { | ||
| const officialV340Default = "{{INBOUND}}|📊{{TRAFFIC_LEFT}}|⏳{{DAYS_LEFT}}D" | ||
| const officialV341Default = "{{INBOUND}}-{{EMAIL}}|📊{{TRAFFIC_LEFT}}|⏳{{DAYS_LEFT}}D" | ||
| return db.Model(&model.Setting{}). | ||
| Where("key = ? AND value = ?", "remarkTemplate", officialV340Default). | ||
| Update("value", officialV341Default).Error | ||
| } |
There was a problem hiding this comment.
🔴 This migration re-runs on every InitDB with no one-shot guard, which makes one legitimate template string permanently unsettable — the opposite of what the comment claims.
Every other value-rewriting settings migration in this file registers a HistoryOfSeeders row so it can never run twice — normalizeFreedomFinalRules → "FreedomFinalRulesReverseFix" (db.go:1658-1682), clearLegacyProxySettings → "LegacyProxySettingsCleanup", seedHostsFromExternalProxy. This one is called unconditionally from initModels:
Lines 171 to 177 in 497a257
remarkTemplate is operator-editable (Settings → Subscription, entity.AllSetting.RemarkTemplate), and UpdateAllSetting writes every field with no skip-if-equals-default, so the row always exists after one save. An operator who deletes -{{EMAIL}} from the shipped default — the single most likely edit, since the email is usually a username that shows up in every client app — lands on a string byte-identical to officialV340Default and gets it silently reverted on the next restart, x-ui setting … invocation, or DB import. Then again, forever. So "operator-customized templates are preserved" is false for exactly the value this function targets.
Note the head branch already ships the v3.4.1 string as DefaultRemarkTemplate (setting.go:39), so this only realigns rows persisted under the older constant — a strictly one-shot job. Gate it the same way the neighbours are gated:
// migrateRemarkTemplateDefault moves panels that persisted the v3.4.0 default
// subscription template to the current default. Exact-match, one-shot.
func migrateRemarkTemplateDefault() error {
const seederName = "RemarkTemplateV341Default"
var seeded int64
if err := db.Model(&model.HistoryOfSeeders{}).Where("seeder_name = ?", seederName).Count(&seeded).Error; err != nil {
return err
}
if seeded > 0 {
return nil
}
const officialV340Default = "{{INBOUND}}|📊{{TRAFFIC_LEFT}}|⏳{{DAYS_LEFT}}D"
const officialV341Default = "{{INBOUND}}-{{EMAIL}}|📊{{TRAFFIC_LEFT}}|⏳{{DAYS_LEFT}}D"
if err := db.Model(&model.Setting{}).
Where("key = ? AND value = ?", "remarkTemplate", officialV340Default).
Update("value", officialV341Default).Error; err != nil {
return err
}
return db.Create(&model.HistoryOfSeeders{SeederName: seederName}).Error
}Also worth a test: TestMigrateRemarkTemplateDefaultPreservesCustomValue seeds {{INBOUND}}/{{EMAIL}}, which is not the value that actually gets clobbered, so it cannot catch this.
| func (s *SubService) uniqueBodyRemark(remark string) string { | ||
| if !s.subscriptionBody || strings.TrimSpace(remark) == "" { | ||
| return remark | ||
| } | ||
| if s.remarkCounts == nil { | ||
| s.remarkCounts = map[string]int{} | ||
| } | ||
| s.remarkCounts[remark]++ | ||
| if s.remarkCounts[remark] <= 1 { | ||
| return remark | ||
| } | ||
| return ctx.configName() | ||
| return fmt.Sprintf("%s-%d", remark, s.remarkCounts[remark]) | ||
| } |
There was a problem hiding this comment.
🔴 The counter is charged at remark-generation time, but callers decide whether to emit the entry afterwards — so dropped links still consume a number and surviving entries ship a -2 with no -1. The composed suffix is also never registered, so it can still collide.
Phantom counts. In the Clash path the remark is built before the bail-outs:
3x-ui/internal/sub/clash_service.go
Lines 237 to 246 in 497a257
buildProxy then returns nil for default: (mtproto / amneziawg / …) and for a failed applyTransport/applySecurity, and the append is skipped. getInboundsBySubId explicitly selects wireguard/amneziawg/mtproto alongside the rest, so these reach the code. Concretely: one subscriber, three enabled inbounds all remarked DE — VLESS/tcp, AmneziaWG, VLESS/ws. The AmneziaWG proxy is dropped but takes count 1, so the surviving ws proxy is emitted as DE-2 with no DE anywhere. On the base branch it was DE. Same shape in the JSON sub (json_service.go:488 vs the continue at 536-539) and in the raw body, where genWireguardLink/genAmneziaWGLink call genRemark after renderHostRemark already charged for the host — one emitted link, two increments, so the very first such link is named …-2. genMtprotoLink emits no remark at all yet still charges.
Collision. fmt.Sprintf("%s-%d", …) is never written back into remarkCounts, so DE, DE, DE-2 emits DE, DE-2, DE-2 — a duplicate, the exact thing this exists to prevent. The correct pattern already lives in the same package and loops until the composed name is unused:
3x-ui/internal/sub/clash_service.go
Lines 127 to 144 in 497a257
That is also why the Clash path needs no help here at all: ensureUniqueProxyNames runs at clash_service.go:75, after all drops, on surviving proxies only, using the identical -N format. So for Clash this change adds nothing and only introduces the phantom.
Suggested shape: drop the uniqueBodyRemark call from the Clash path, and for raw/JSON either move the charge to the point the entry is known to survive, or at minimum register the composed name and loop until unused. The new tests (remark_vars_test.go:778-830) call genHostRemark/genTemplatedRemark directly and cannot see the drop interaction — a test that drives a real subscription containing a droppable inbound would.
| {messageContextHolder} | ||
| <Modal | ||
| open={open} | ||
| title={t('pages.inbounds.relay.title', { defaultValue: '新建中转' })} |
There was a problem hiding this comment.
🟡 The whole relay wizard and preset gallery render in Simplified Chinese for all 13 locales — ~45 strings whose keys exist in none of the locale files.
pages.inbounds.relay.title is not in any of the 13 files under internal/web/translation/; the only relay child added by this PR is badge. Same for every pages.inbounds.presets.* and recommendToggle* key (grep presets|recommendToggle over the locale dir → 0 matches). i18next is initialised with no parseMissingKeyHandler, so a key missing from both the active bundle and the en-US fallback returns options.defaultValue — the Chinese literal is what an English, Russian or Persian operator actually sees.
Affected: RelayWizardModal.tsx (34 sites), InboundFormModal.tsx (11), PRESET_FALLBACK in inbound-presets.ts#L285-L308, and the toolbar button at InboundList.tsx#L206-L210 — plus three with no t() at all (placeholder="客户端 UUID" at :321, placeholder="1.2.3.4 或 …" at :313, and the 、 list separator in the InboundFormModal.tsx:481 toast). Line 168 uses '中转' as the stored remark when the field is blank, so the Chinese also reaches the DB and the share link.
This is not the "missing i18n key" case CI covers: i18n-dead-keys.test.ts only asserts (a) every en-US key is referenced and (b) locale sets match en-US. Both pass here, because the keys were never added to en-US. Nothing else in ci.yml checks that a referenced key resolves — so this ships silently.
The base branch has zero CJK defaultValue literals (its 6 defaultValue: uses are all t(err.message, { defaultValue: err.message })), so this introduces the pattern rather than following it. Add the ~51 keys to en-US.json and the other 12 locales and drop the Chinese defaults.
| const onLandingAddressChange = (value: string) => { | ||
| const parsed = parseLandingEndpoint(value); | ||
| if (!parsed) { | ||
| setAddress(value); | ||
| return; | ||
| } | ||
| setAddress(parsed.address); | ||
| if (parsed.port) setLandingPort(parsed.port); | ||
| if (parsed.user !== undefined) setUser(parsed.user); | ||
| if (parsed.pass !== undefined) setPass(parsed.pass); | ||
| }; |
There was a problem hiding this comment.
🟡 This rewrites a controlled input on every keystroke, so typing host:port (rather than pasting it) mangles the address and pins the port to the first digit typed.
The comment says "when the user pastes", but the handler is wired to onChange on a controlled <Input value={address}> (:310-314), which fires per character. Typing 1.2.3.4:1080:
| typed | value in | parse | field becomes |
|---|---|---|---|
1.2.3.4 |
1.2.3.4 |
null | 1.2.3.4 |
: |
1.2.3.4: |
null | 1.2.3.4: |
1 |
1.2.3.4:1 |
{address:'1.2.3.4', port:1} |
1.2.3.4 — the :1 is deleted, port := 1 |
0 |
1.2.3.40 |
null | 1.2.3.40 |
8 0 |
… | null | 1.2.3.4080 |
End state: address 1.2.3.4080, port 1. example.com:8080 ends as example.com080 / port 8. Those values go straight into buildLanding → the outbound that create() splices into the live Xray config and restarts on. Pasting the whole string in one event is fine — only typing breaks.
Simplest fix: only auto-split when the value arrives as a paste (onPaste), or gate the rewrite behind onBlur so the user's in-progress text is never replaced mid-edit.
| func (s *InboundService) DelInbounds(ids []int, purgeClients bool) (BulkDelInboundResult, bool, error) { | ||
| result := BulkDelInboundResult{} | ||
| needRestart := false | ||
| if purgeClients { | ||
| restart, err := s.clientService.PurgeOrphansForInbounds(s, ids) | ||
| if err != nil { | ||
| return result, needRestart, err | ||
| } | ||
| needRestart = restart | ||
| } |
There was a problem hiding this comment.
🟡 The purge runs before any inbound is deleted and nothing wraps the two, so a failure permanently deletes clients while leaving the inbounds in place.
PurgeOrphansForInbounds loops s.Delete(…) and returns on the first error (client_link.go#L324-L341); each client delete commits its own transaction, so everything before the failure is already gone. Two ways it bites:
- Purge fails partway → the early
returnat line 1324 fires before the delete loop, so N clients are permanently gone (record, traffic, IP log, HWID, external links) and zero inbounds are deleted. Reachable vianodePushPlanreturninggorm.ErrRecordNotFoundfor a node-backed inbound whosenodesrow is missing, or the persistent"invalid clients format in inbound settings"atclient_inbound_apply.go:1045-1052— persistent meaning the operator loses more clients on every retry. - A later
DelInbound(id)fails → this function treats that as expected and records it inSkipped(that is what the doc comment above says), but the purge already destroyed the orphans of every id including the skipped one. That inbound survives, stripped of its clients, and the API still returns 200 with a partial-success body.
Computing the orphan ids first, deleting the inbounds, then purging only the ids that actually got deleted fails safe in both directions. Same ordering in DelInboundPurgeOrphans at :1291-1302.
Minor, same lines: PurgeOrphansForInbounds can return (true, err) — client_inbound_apply.go:1167/1175 sets needRestart when a live client's hot removal could not be applied — but line 1324 returns the still-false local, and 1296 returns a literal false. A client removed from the DB then keeps working in the running Xray until an unrelated restart. (Both controller call sites also drop needRestart on error, so fixing only these two returns changes nothing observable — worth doing together.)
There are also no tests for PurgeOrphansForInbounds, CountOrphansForInbounds or DelInboundPurgeOrphans; nothing in the tree references them.
| } | ||
|
|
||
| func TestIdentityTokenBodyVsDisplay(t *testing.T) { | ||
| func TestIdentityTokenKeptInBodyAndDisplay(t *testing.T) { |
There was a problem hiding this comment.
🟡 Both rewritten tests pass identically on the base branch and on this branch, so neither can fail without the change they accompany.
The production edit they follow is a pure no-op rename: base had mergeTokenSets(usageInfoTokens, map[string]bool{"EMAIL": true, "USERNAME": true}), head has mergeTokenSets(usageInfoTokens, identityTokens) with identityTokens being that identical map. showIdentityOnAllLinks and effectiveTemplate are unchanged from base. So the commit "fix: honor subscription identity toggle" changes no behaviour.
Setting showIdentityOnAllLinks: true here (and in TestEmailOnEveryBodyLink at :684-713) then flipping the assertions makes both tests duplicates of the already-existing TestIdentityOnAllLinks/enabled subtest, which the PR leaves untouched and which asserts exact equality rather than strings.Contains — strictly stronger:
3x-ui/internal/sub/remark_vars_test.go
Lines 730 to 749 in 497a257
Per CLAUDE.md: "A test must fail without its fix. … A test that passes either way is worse than no test." I checked and the original versions of both tests also still pass against this branch's code, so nothing forced the rewrite. Default-path coverage survives via TestIdentityOnAllLinks/disabled and TestSharedSubIDRemark_FullInfoOncePerSubscription, so the cleanest fix is to revert both tests and drop the no-op identityTokens extraction with them.
| // One-click inbound presets. Each preset turns a single click into a full, | ||
| // ready-to-save inbound config: protocol + transport + security + one default | ||
| // client + a random port. The modal feeds the returned row through | ||
| // rawInboundToFormValues() — the exact same path buildAddModeValues() uses — | ||
| // so every preset rides the existing form validation and submit pipeline. | ||
| // | ||
| // Two flags drive post-apply work the modal owns: | ||
| // - needsRealityKeys: the modal fetches GET /panel/api/server/getNewX25519Cert | ||
| // and injects privateKey/publicKey after applying (Reality keys are | ||
| // generated by the xray binary, not in the browser). | ||
| // - needsDomain: the preset is TLS-based and the share link won't work until | ||
| // the operator points a domain at this server and installs a cert. The | ||
| // modal surfaces a domain input; the typed value lands in serverName/SNI. |
There was a problem hiding this comment.
🟡 19 new comment blocks in this PR exceed the 2-line cap — CLAUDE.md: "Comments in committed Go/TS: 2 lines MAX per comment block. … spend the 2 lines on the why a name cannot hold."
This one is 13 lines. Counts by file: inbound-presets.ts 6 (this block plus :63, :82, :99, :158, :242), relay.ts 9 (longest is 8 lines at :49), RelayWizardModal.tsx 2, internal/database/db.go 1 (:392), and frontend/src/lib/inbounds/label.ts where the PR lengthened a previously-compliant 2-line block to 4. None are directives or HTML comments.
Flagging once with the count rather than 19 times. Worth noting the base branch is not clean here either (outbound-link-parser.ts opens with a 15-line header, internal/xray/process.go has ~20 such blocks), so this is a low-priority tidy-up — but the new files are where the rule is cheapest to honour. firstLinkOnlyBodyTokens in remark_vars.go went 4 → 3 lines in this PR, which is the right direction.
Code review5 🔴 / 5 🟡 / 1 🟣 (plus 3 similar nits not posted) Detail is in the inline comments; this is the map. 🔴 Important
🟡 Nits
Not posted, same shape: hysteria2 preset leaves 🟣 Pre-existing
Coverage
|
1 participant
Summary
Why
Type of change
Areas affected
How was this tested?
Screenshots / recordings
Breaking changes
Checklist
go build ./...and the test suite pass locally.npm run lint,npm run typecheck, andnpm run buildpass.