From 878ee839dbe574021123f6adbf35eac6d768c9b2 Mon Sep 17 00:00:00 2001 From: Kuzz007 Date: Tue, 4 Aug 2026 14:13:20 +0300 Subject: [PATCH] Split the client edit form's AllowedIPs into per-protocol fields A client attached to both WireGuard and AmneziaWG shared one AllowedIPs form field with a dynamically-switching label, so its two genuinely different addresses could never both be shown or edited correctly. Worse, Update/Create broadcast that one shared value to every attached wg/awg inbound with no subnet-fit check, so an ordinary edit save could silently overwrite one protocol's address with the other's -- the same bug class already fixed for Attach, but reachable from any client edit. model.Client gains an optional AllowedIPsByInbound map so a caller can send distinct values per inbound; Update/Create honor it and, when it's absent, clear a shared value that doesn't fit an AmneziaWG inbound's own subnet instead of writing it through. A new TunnelAllowedIPsByInbound read path feeds the real per-inbound address to the client edit form via GET, which now renders two separate, correctly-labeled fields whenever both protocols are attached (unchanged single dynamic field otherwise). --- frontend/src/generated/examples.ts | 1 + frontend/src/generated/schemas.ts | 10 + frontend/src/generated/types.ts | 1 + frontend/src/generated/zod.ts | 1 + .../src/pages/clients/ClientFormModal.tsx | 132 ++++++++--- frontend/src/pages/clients/ClientsPage.tsx | 4 + frontend/src/schemas/client.ts | 7 + .../test/client-tunnel-allowed-ips.test.tsx | 54 +++++ internal/database/model/model.go | 60 ++--- internal/web/controller/client.go | 13 +- internal/web/service/client_crud.go | 31 ++- internal/web/service/client_lookup.go | 35 +++ .../service/client_update_allowedips_test.go | 213 ++++++++++++++++++ 13 files changed, 500 insertions(+), 62 deletions(-) create mode 100644 frontend/src/test/client-tunnel-allowed-ips.test.tsx create mode 100644 internal/web/service/client_update_allowedips_test.go diff --git a/frontend/src/generated/examples.ts b/frontend/src/generated/examples.ts index 1da8e260e..546ed0b4b 100644 --- a/frontend/src/generated/examples.ts +++ b/frontend/src/generated/examples.ts @@ -236,6 +236,7 @@ export const EXAMPLES: Record = { "allowedIPs": [ "" ], + "allowedIPsByInbound": {}, "auth": "", "comment": "", "created_at": 0, diff --git a/frontend/src/generated/schemas.ts b/frontend/src/generated/schemas.ts index 7db45c656..0e2a7c51f 100644 --- a/frontend/src/generated/schemas.ts +++ b/frontend/src/generated/schemas.ts @@ -1002,6 +1002,16 @@ export const SCHEMAS: Record = { }, "type": "array" }, + "allowedIPsByInbound": { + "additionalProperties": { + "items": { + "type": "string" + }, + "type": "array" + }, + "description": "AllowedIPsByInbound optionally overrides AllowedIPs on a per-inbound\nbasis, keyed by inbound id. Lets one identity attached to both\nWireGuard and AmneziaWG carry two genuinely different addresses in a\nsingle Create/Update call instead of the shared AllowedIPs field\nbeing broadcast to every attached tunnel inbound. Absent/unset for a\ngiven inbound id falls back to the shared AllowedIPs exactly as\nbefore -- fully backward compatible for callers that never set this.", + "type": "object" + }, "auth": { "description": "Auth password (Hysteria)", "type": "string" diff --git a/frontend/src/generated/types.ts b/frontend/src/generated/types.ts index 2332a0c1b..2271a54a6 100644 --- a/frontend/src/generated/types.ts +++ b/frontend/src/generated/types.ts @@ -245,6 +245,7 @@ export interface ApiTokenView { export interface Client { adTag?: string; allowedIPs?: string[]; + allowedIPsByInbound?: Record; auth?: string; comment: string; created_at?: number; diff --git a/frontend/src/generated/zod.ts b/frontend/src/generated/zod.ts index 3e510ee98..bd51118ab 100644 --- a/frontend/src/generated/zod.ts +++ b/frontend/src/generated/zod.ts @@ -263,6 +263,7 @@ export type ApiTokenView = z.infer; export const ClientSchema = z.object({ adTag: z.string().optional(), allowedIPs: z.array(z.string()).optional(), + allowedIPsByInbound: z.record(z.number().int(), z.array(z.string())).optional(), auth: z.string().optional(), comment: z.string(), created_at: z.number().int().optional(), diff --git a/frontend/src/pages/clients/ClientFormModal.tsx b/frontend/src/pages/clients/ClientFormModal.tsx index 5a05522fb..8f1f1e6e8 100644 --- a/frontend/src/pages/clients/ClientFormModal.tsx +++ b/frontend/src/pages/clients/ClientFormModal.tsx @@ -85,6 +85,7 @@ interface ClientFormModalProps { inbounds: InboundOption[]; attachedExternalLinks?: ExternalLink[]; attachedIds?: number[]; + tunnelAllowedIPs?: Record; tgBotEnable?: boolean; groups?: string[]; save: ( @@ -102,6 +103,7 @@ type Values = ClientFormValues & { wgPublicKey: string; wgPreSharedKey: string; wgAllowedIPs: string; + awgAllowedIPs: string; awgForwardedPorts: string; secret: string; adTag: string; @@ -132,6 +134,7 @@ const EMPTY: Values = { wgPublicKey: '', wgPreSharedKey: '', wgAllowedIPs: '', + awgAllowedIPs: '', awgForwardedPorts: '', secret: '', adTag: '', @@ -155,6 +158,31 @@ export function gbToBytes(gb: number): number { return Math.round(gb * 1024 * 1024 * 1024); } +export function parseAllowedIPsList(raw: string): string[] { + return raw.split(',').map((s) => s.trim()).filter((s) => s !== ''); +} + +// Maps each of the two AllowedIPs fields to the specific wg/awg inbound the +// client is currently attached to, so a save with both protocols attached at +// once can send each its own value instead of one shared field ambiguously +// covering both (see model.Client.AllowedIPsByInbound on the Go side). +// Absent from the result when the client isn't actually attached to that +// protocol's inbound (e.g. mid-edit, before the attach takes effect). +export function resolveTunnelAllowedIPsByInbound( + attachedInboundIds: number[], + wireguardInboundIds: Set, + amneziawgInboundIds: Set, + wgAllowedIPs: string[], + awgAllowedIPs: string[], +): Record { + const wgId = attachedInboundIds.find((id) => wireguardInboundIds.has(id)); + const awgId = attachedInboundIds.find((id) => amneziawgInboundIds.has(id)); + const result: Record = {}; + if (wgId != null) result[wgId] = wgAllowedIPs; + if (awgId != null) result[awgId] = awgAllowedIPs; + return result; +} + export function resolveTotalBytes(originalBytes: number | null | undefined, displayedGB: number): number { if (originalBytes != null && displayedGB === bytesToGB(originalBytes)) { return originalBytes; @@ -169,6 +197,7 @@ export default function ClientFormModal({ inbounds, attachedExternalLinks = [], attachedIds = [], + tunnelAllowedIPs = {}, tgBotEnable = false, groups = [], save, @@ -210,6 +239,27 @@ export default function ClientFormModal({ const limitIpDisabled = !fail2ban.usable; const limitIpNotice = getLimitIpNotice(fail2ban, t); + // Declared ahead of the seeding effect below (which needs them to resolve + // which specific wg/awg inbound this client is attached to, for seeding + // wgAllowedIPs/awgAllowedIPs from tunnelAllowedIPs) -- both are pure + // derivations of the stable `inbounds` prop, so moving them earlier is + // just a declaration-order change, not a behavior change. + const wireguardIds = useMemo(() => { + const ids = new Set(); + for (const row of inbounds || []) { + if (row && row.protocol === 'wireguard') ids.add(row.id); + } + return ids; + }, [inbounds]); + + const amneziawgIds = useMemo(() => { + const ids = new Set(); + for (const row of inbounds || []) { + if (row && row.protocol === 'amneziawg') ids.add(row.id); + } + return ids; + }, [inbounds]); + function addExternalLinkRow(kind: 'link' | 'subscription') { appendExternalLink({ kind, value: '', remark: '' }); } @@ -220,6 +270,11 @@ export default function ClientFormModal({ if (isEdit && client) { const et = Number(client.expiryTime) || 0; + const seedIds = Array.isArray(attachedIds) ? attachedIds : []; + const attachedWireguardId = seedIds.find((id) => wireguardIds.has(id)); + const attachedAmneziawgId = seedIds.find((id) => amneziawgIds.has(id)); + const wgTunnelIPs = attachedWireguardId != null ? tunnelAllowedIPs[attachedWireguardId] : undefined; + const awgTunnelIPs = attachedAmneziawgId != null ? tunnelAllowedIPs[attachedAmneziawgId] : undefined; const seed: Values = { ...EMPTY, email: client.email || '', @@ -244,7 +299,8 @@ export default function ClientFormModal({ wgPrivateKey: client.privateKey || '', wgPublicKey: client.publicKey || '', wgPreSharedKey: client.preSharedKey || '', - wgAllowedIPs: client.allowedIPs || '', + wgAllowedIPs: wgTunnelIPs ?? client.allowedIPs ?? '', + awgAllowedIPs: awgTunnelIPs ?? client.allowedIPs ?? '', awgForwardedPorts: client.forwardedPorts || '', secret: client.secret || '', adTag: client.adTag || '', @@ -301,22 +357,6 @@ export default function ClientFormModal({ return ids; }, [inbounds]); - const wireguardIds = useMemo(() => { - const ids = new Set(); - for (const row of inbounds || []) { - if (row && row.protocol === 'wireguard') ids.add(row.id); - } - return ids; - }, [inbounds]); - - const amneziawgIds = useMemo(() => { - const ids = new Set(); - for (const row of inbounds || []) { - if (row && row.protocol === 'amneziawg') ids.add(row.id); - } - return ids; - }, [inbounds]); - const mtprotoIds = useMemo(() => { const ids = new Set(); for (const row of inbounds || []) { @@ -554,12 +594,25 @@ export default function ClientFormModal({ if (values.wgPreSharedKey) { clientPayload.preSharedKey = values.wgPreSharedKey; } - const allowedIPs = values.wgAllowedIPs - .split(',') - .map((s) => s.trim()) - .filter((s) => s !== ''); - if (allowedIPs.length > 0) { - clientPayload.allowedIPs = allowedIPs; + const wgAllowedIPs = parseAllowedIPsList(values.wgAllowedIPs); + if (showWireguard && showAmneziawg) { + // Both protocols are attached at once: the two fields hold genuinely + // different addresses, so each must land on its own inbound instead + // of one broadcast value overwriting the other's (allowedIPsByInbound + // is what Update/Create key their per-inbound override off of). + const awgAllowedIPs = parseAllowedIPsList(values.awgAllowedIPs); + clientPayload.allowedIPsByInbound = resolveTunnelAllowedIPsByInbound( + values.inboundIds || [], + wireguardIds, + amneziawgIds, + wgAllowedIPs, + awgAllowedIPs, + ); + if (wgAllowedIPs.length > 0) { + clientPayload.allowedIPs = wgAllowedIPs; + } + } else if (wgAllowedIPs.length > 0) { + clientPayload.allowedIPs = wgAllowedIPs; } // Port-forwarding has no WireGuard equivalent — Xray-native WireGuard // has no host-level iptables layer to hang per-client DNAT off of. @@ -901,13 +954,32 @@ export default function ClientFormModal({ > - - - + {showWireguard && showAmneziawg ? ( + <> + + + + + + + + ) : ( + + + + )} {showAmneziawg && ( (null); const [editingAttachedIds, setEditingAttachedIds] = useState([]); const [editingExternalLinks, setEditingExternalLinks] = useState([]); + const [editingTunnelAllowedIPs, setEditingTunnelAllowedIPs] = useState>({}); const [infoOpen, setInfoOpen] = useState(false); const [infoClient, setInfoClient] = useState(null); const [qrOpen, setQrOpen] = useState(false); @@ -503,6 +504,7 @@ export default function ClientsPage() { setEditingClient(null); setEditingAttachedIds([]); setEditingExternalLinks([]); + setEditingTunnelAllowedIPs({}); setFormOpen(true); } @@ -518,6 +520,7 @@ export default function ClientsPage() { const ids = full?.inboundIds ?? (Array.isArray(row.inboundIds) ? row.inboundIds : []); setEditingAttachedIds([...ids]); setEditingExternalLinks(Array.isArray(full?.externalLinks) ? [...full.externalLinks] : []); + setEditingTunnelAllowedIPs(full?.tunnelAllowedIPs ?? {}); setFormOpen(true); }, [hydrate]); @@ -1506,6 +1509,7 @@ export default function ClientsPage() { client={editingClient} attachedIds={editingAttachedIds} attachedExternalLinks={editingExternalLinks} + tunnelAllowedIPs={editingTunnelAllowedIPs} inbounds={inbounds} tgBotEnable={tgBotEnable} groups={allGroups} diff --git a/frontend/src/schemas/client.ts b/frontend/src/schemas/client.ts index 062710cb0..a99232162 100644 --- a/frontend/src/schemas/client.ts +++ b/frontend/src/schemas/client.ts @@ -133,10 +133,17 @@ export const ExternalLinkSchema = z.object({ export const ExternalLinkListSchema = z.array(ExternalLinkSchema).nullable().transform((v) => v ?? []); +// tunnelAllowedIPs carries the real, per-inbound AllowedIPs value (keyed by +// inbound id) for every WireGuard/AmneziaWG inbound this client is attached +// to. ClientRecord's own allowedIPs is a single string and cannot represent +// two different addresses when one identity holds both a WireGuard and an +// AmneziaWG attachment at once -- this is what lets the edit form show each +// protocol's real, distinct address instead of one ambiguous shared field. export const ClientHydrateSchema = z.object({ client: ClientRecordSchema, inboundIds: nullableNumberArray, externalLinks: ExternalLinkListSchema.optional(), + tunnelAllowedIPs: z.record(z.number().int(), z.string()).optional(), }); export const BulkAdjustResultSchema = z.object({ diff --git a/frontend/src/test/client-tunnel-allowed-ips.test.tsx b/frontend/src/test/client-tunnel-allowed-ips.test.tsx new file mode 100644 index 000000000..67377bc30 --- /dev/null +++ b/frontend/src/test/client-tunnel-allowed-ips.test.tsx @@ -0,0 +1,54 @@ +import { describe, it, expect } from 'vitest'; + +import { parseAllowedIPsList, resolveTunnelAllowedIPsByInbound } from '@/pages/clients/ClientFormModal'; + +describe('parseAllowedIPsList', () => { + it('splits, trims, and drops empty entries', () => { + expect(parseAllowedIPsList(' 10.0.0.2/32 , 10.0.0.3/32,')).toEqual(['10.0.0.2/32', '10.0.0.3/32']); + }); + + it('returns an empty array for a blank string', () => { + expect(parseAllowedIPsList('')).toEqual([]); + }); +}); + +describe('resolveTunnelAllowedIPsByInbound', () => { + // Regression coverage for the bug this whole feature exists to fix: a + // client attached to both a WireGuard and an AmneziaWG inbound must get + // each protocol's own address routed to its own inbound id, never the + // other's -- a single shared field can't represent two different + // addresses, which is exactly what confused wg's 10.0.0.2/32 with awg's + // 10.8.1.0/24 subnet in the real production bug report. + it('maps each protocol field to its own attached inbound id', () => { + const wireguardIds = new Set([7]); + const amneziawgIds = new Set([10]); + const result = resolveTunnelAllowedIPsByInbound( + [7, 10], + wireguardIds, + amneziawgIds, + ['10.0.0.2/32'], + ['10.8.1.21/32'], + ); + expect(result).toEqual({ 7: ['10.0.0.2/32'], 10: ['10.8.1.21/32'] }); + }); + + it('omits a protocol entirely when its inbound is not among the attached ids', () => { + const wireguardIds = new Set([7]); + const amneziawgIds = new Set([10]); + const result = resolveTunnelAllowedIPsByInbound([7], wireguardIds, amneziawgIds, ['10.0.0.2/32'], ['10.8.1.21/32']); + expect(result).toEqual({ 7: ['10.0.0.2/32'] }); + expect(result).not.toHaveProperty('10'); + }); + + it('returns an empty object when neither protocol is attached', () => { + const result = resolveTunnelAllowedIPsByInbound([3], new Set([7]), new Set([10]), ['x'], ['y']); + expect(result).toEqual({}); + }); + + it('picks the first matching id when multiple inbounds of the same protocol are attached', () => { + const wireguardIds = new Set([7, 8]); + const amneziawgIds = new Set([10]); + const result = resolveTunnelAllowedIPsByInbound([8, 7, 10], wireguardIds, amneziawgIds, ['10.0.0.2/32'], ['10.8.1.21/32']); + expect(result).toEqual({ 8: ['10.0.0.2/32'], 10: ['10.8.1.21/32'] }); + }); +}); diff --git a/internal/database/model/model.go b/internal/database/model/model.go index 4df146864..1eedaedcc 100644 --- a/internal/database/model/model.go +++ b/internal/database/model/model.go @@ -858,32 +858,40 @@ type ClientReverse struct { // Client represents a client configuration for Xray inbounds with traffic limits and settings. type Client struct { - ID string `json:"id,omitempty"` // Unique client identifier - Security string `json:"security"` // Security method (e.g., "auto", "aes-128-gcm") - Password string `json:"password,omitempty"` // Client password - Flow string `json:"flow,omitempty"` // Flow control (XTLS) - Reverse *ClientReverse `json:"reverse,omitempty"` // VLESS simple reverse proxy settings - Auth string `json:"auth,omitempty"` // Auth password (Hysteria) - PrivateKey string `json:"privateKey,omitempty"` - PublicKey string `json:"publicKey,omitempty"` - AllowedIPs []string `json:"allowedIPs,omitempty"` - PreSharedKey string `json:"preSharedKey,omitempty"` - KeepAlive int `json:"keepAlive,omitempty"` - ForwardedPorts string `json:"forwardedPorts,omitempty"` // AmneziaWG per-client port-forwarding spec, e.g. "80,443,8000-8100" - Secret string `json:"secret,omitempty" example:"ee1234567890abcdef1234567890abcd7777772e636c6f7564666c6172652e636f6d"` - AdTag string `json:"adTag,omitempty" example:"0123456789abcdef0123456789abcdef"` - Email string `json:"email"` // Client email identifier - LimitIP int `json:"limitIp"` // IP limit for this client - TotalGB int64 `json:"totalGB" form:"totalGB"` // Total traffic limit in GB - ExpiryTime int64 `json:"expiryTime" form:"expiryTime"` // Expiration timestamp - Enable bool `json:"enable" form:"enable"` // Whether the client is enabled - TgID int64 `json:"tgId" form:"tgId"` // Telegram user ID for notifications - SubID string `json:"subId" form:"subId"` // Subscription identifier - Group string `json:"group,omitempty" form:"group"` // Logical grouping label - Comment string `json:"comment" form:"comment"` // Client comment - Reset int `json:"reset" form:"reset"` // Reset period in days - CreatedAt int64 `json:"created_at,omitempty"` // Creation timestamp - UpdatedAt int64 `json:"updated_at,omitempty"` // Last update timestamp + ID string `json:"id,omitempty"` // Unique client identifier + Security string `json:"security"` // Security method (e.g., "auto", "aes-128-gcm") + Password string `json:"password,omitempty"` // Client password + Flow string `json:"flow,omitempty"` // Flow control (XTLS) + Reverse *ClientReverse `json:"reverse,omitempty"` // VLESS simple reverse proxy settings + Auth string `json:"auth,omitempty"` // Auth password (Hysteria) + PrivateKey string `json:"privateKey,omitempty"` + PublicKey string `json:"publicKey,omitempty"` + AllowedIPs []string `json:"allowedIPs,omitempty"` + // AllowedIPsByInbound optionally overrides AllowedIPs on a per-inbound + // basis, keyed by inbound id. Lets one identity attached to both + // WireGuard and AmneziaWG carry two genuinely different addresses in a + // single Create/Update call instead of the shared AllowedIPs field + // being broadcast to every attached tunnel inbound. Absent/unset for a + // given inbound id falls back to the shared AllowedIPs exactly as + // before -- fully backward compatible for callers that never set this. + AllowedIPsByInbound map[int][]string `json:"allowedIPsByInbound,omitempty"` + PreSharedKey string `json:"preSharedKey,omitempty"` + KeepAlive int `json:"keepAlive,omitempty"` + ForwardedPorts string `json:"forwardedPorts,omitempty"` // AmneziaWG per-client port-forwarding spec, e.g. "80,443,8000-8100" + Secret string `json:"secret,omitempty" example:"ee1234567890abcdef1234567890abcd7777772e636c6f7564666c6172652e636f6d"` + AdTag string `json:"adTag,omitempty" example:"0123456789abcdef0123456789abcdef"` + Email string `json:"email"` // Client email identifier + LimitIP int `json:"limitIp"` // IP limit for this client + TotalGB int64 `json:"totalGB" form:"totalGB"` // Total traffic limit in GB + ExpiryTime int64 `json:"expiryTime" form:"expiryTime"` // Expiration timestamp + Enable bool `json:"enable" form:"enable"` // Whether the client is enabled + TgID int64 `json:"tgId" form:"tgId"` // Telegram user ID for notifications + SubID string `json:"subId" form:"subId"` // Subscription identifier + Group string `json:"group,omitempty" form:"group"` // Logical grouping label + Comment string `json:"comment" form:"comment"` // Client comment + Reset int `json:"reset" form:"reset"` // Reset period in days + CreatedAt int64 `json:"created_at,omitempty"` // Creation timestamp + UpdatedAt int64 `json:"updated_at,omitempty"` // Last update timestamp } type ClientRecord struct { diff --git a/internal/web/controller/client.go b/internal/web/controller/client.go index 827b5bd81..c66ecb14d 100644 --- a/internal/web/controller/client.go +++ b/internal/web/controller/client.go @@ -124,11 +124,16 @@ func (a *ClientController) buildClientPayload(rec *model.ClientRecord) (gin.H, e if t, tErr := a.inboundService.GetClientTrafficByEmail(rec.Email); tErr == nil && t != nil { usedTraffic = t.Up + t.Down } + tunnelAllowedIPs, err := a.clientService.TunnelAllowedIPsByInbound(&a.inboundService, rec.Email, inboundIds) + if err != nil { + return nil, err + } return gin.H{ - "client": rec, - "inboundIds": inboundIds, - "externalLinks": externalLinks, - "usedTraffic": usedTraffic, + "client": rec, + "inboundIds": inboundIds, + "externalLinks": externalLinks, + "usedTraffic": usedTraffic, + "tunnelAllowedIPs": tunnelAllowedIPs, }, nil } diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index 5656920e1..fd6d002f0 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -126,7 +126,19 @@ func (s *ClientService) Create(inboundSvc *InboundService, payload *ClientCreate if err := s.fillProtocolDefaults(&client, inbound); err != nil { return needRestart, err } - settingsPayload, mErr := json.Marshal(map[string][]model.Client{"clients": {clientWithInboundFlow(client, inbound)}}) + clientForInbound := client + if ips, ok := client.AllowedIPsByInbound[ibId]; ok { + clientForInbound.AllowedIPs = ips + } else if !addressesFitAmneziaWGInbound(clientForInbound.AllowedIPs, inbound) { + // The shared AllowedIPs value (e.g. from a single-field legacy + // caller) came from a different subnet than this inbound's own -- + // clear it so defaultAmneziaWGClients allocates a fresh, correct + // address for THIS inbound instead of persisting an unroutable + // peer. Same reasoning as addressesFitAmneziaWGInbound's own doc + // comment on the Attach path. + clientForInbound.AllowedIPs = nil + } + settingsPayload, mErr := json.Marshal(map[string][]model.Client{"clients": {clientWithInboundFlow(clientForInbound, inbound)}}) if mErr != nil { return needRestart, mErr } @@ -414,7 +426,22 @@ func (s *ClientService) Update(inboundSvc *InboundService, id int, updated model if err := s.fillProtocolDefaults(&updated, inbound); err != nil { return needRestart, err } - settingsPayload, mErr := json.Marshal(map[string][]model.Client{"clients": {clientWithInboundFlow(updated, inbound)}}) + clientForInbound := updated + if ips, ok := updated.AllowedIPsByInbound[ibId]; ok { + clientForInbound.AllowedIPs = ips + } else if !addressesFitAmneziaWGInbound(clientForInbound.AllowedIPs, inbound) { + // A single shared AllowedIPs field (the common case for a caller + // that never sends AllowedIPsByInbound) must never overwrite an + // inbound it doesn't belong to -- e.g. a client attached to both + // wg and awg saving its wg-labeled address would otherwise get + // that same address silently written into the awg peer config + // too. Clearing it here makes UpdateInboundClient's own + // empty-AllowedIPs carry-forward (see its WireGuard/AmneziaWG + // branch) preserve THIS inbound's existing, correct value + // instead. + clientForInbound.AllowedIPs = nil + } + settingsPayload, mErr := json.Marshal(map[string][]model.Client{"clients": {clientWithInboundFlow(clientForInbound, inbound)}}) if mErr != nil { return needRestart, mErr } diff --git a/internal/web/service/client_lookup.go b/internal/web/service/client_lookup.go index dbd9f6d10..5cda43942 100644 --- a/internal/web/service/client_lookup.go +++ b/internal/web/service/client_lookup.go @@ -133,6 +133,41 @@ func (s *ClientService) GetInboundIdsForRecord(id int) ([]int, error) { return ids, nil } +// TunnelAllowedIPsByInbound returns, for each given WireGuard/AmneziaWG +// inbound id, the real AllowedIPs this email currently has on that specific +// inbound's own settings JSON -- joined comma-separated, matching the form +// value shape a single AllowedIPs field already uses. Non-tunnel inbounds +// and ids the email isn't actually attached to are simply absent from the +// result (not an error): callers use this to seed a per-protocol display +// field, and ClientRecord's own single AllowedIPs column can't tell two +// different protocol addresses apart, which is exactly the gap this closes. +func (s *ClientService) TunnelAllowedIPsByInbound(inboundSvc *InboundService, email string, inboundIds []int) (map[int]string, error) { + result := make(map[int]string, len(inboundIds)) + for _, ibId := range inboundIds { + inbound, err := inboundSvc.GetInbound(ibId) + if err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + continue + } + return nil, err + } + if inbound.Protocol != model.WireGuard && inbound.Protocol != model.AmneziaWG { + continue + } + clients, err := inboundSvc.GetClients(inbound) + if err != nil { + return nil, err + } + for i := range clients { + if strings.EqualFold(clients[i].Email, email) { + result[ibId] = strings.Join(clients[i].AllowedIPs, ",") + break + } + } + } + return result, nil +} + func (s *ClientService) List() ([]ClientWithAttachments, error) { db := database.GetDB() var rows []model.ClientRecord diff --git a/internal/web/service/client_update_allowedips_test.go b/internal/web/service/client_update_allowedips_test.go new file mode 100644 index 000000000..ddd3a6512 --- /dev/null +++ b/internal/web/service/client_update_allowedips_test.go @@ -0,0 +1,213 @@ +package service + +import ( + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +// seedDualProtocolClient creates a WireGuard inbound and an AmneziaWG inbound +// (real, distinct subnets: 10.0.0.0/24 and 10.8.1.0/24), attaches the same +// email to both with its own correct, protocol-appropriate address, and +// returns the two inbounds plus the shared client record id. +func seedDualProtocolClient(t *testing.T, email, wgAddr, awgAddr string) (wgIb, awgIb *model.Inbound, recordId int) { + t.Helper() + svc := &ClientService{} + + wgClient := model.Client{Email: email, SubID: "sub-" + email, Enable: true, AllowedIPs: []string{wgAddr}} + wgIb = mkInbound(t, 51820, model.WireGuard, clientsSettings(t, []model.Client{wgClient})) + if err := svc.SyncInbound(nil, wgIb.Id, []model.Client{wgClient}); err != nil { + t.Fatalf("seed wg linkage: %v", err) + } + + awgClient := model.Client{Email: email, SubID: "sub-" + email, Enable: true, AllowedIPs: []string{awgAddr}} + awgIb = mkInbound(t, 443, model.AmneziaWG, clientsSettings(t, []model.Client{awgClient})) + if err := svc.SyncInbound(nil, awgIb.Id, []model.Client{awgClient}); err != nil { + t.Fatalf("seed awg linkage: %v", err) + } + + recordId = lookupClientRecord(t, email).Id + return wgIb, awgIb, recordId +} + +func inboundAllowedIPs(t *testing.T, inboundSvc *InboundService, ibId int, email string) []string { + t.Helper() + ib, err := inboundSvc.GetInbound(ibId) + if err != nil { + t.Fatalf("GetInbound %d: %v", ibId, err) + } + clients, err := inboundSvc.GetClients(ib) + if err != nil { + t.Fatalf("GetClients %d: %v", ibId, err) + } + for i := range clients { + if clients[i].Email == email { + return clients[i].AllowedIPs + } + } + t.Fatalf("email %q not found on inbound %d", email, ibId) + return nil +} + +// TestUpdateBroadcastAllowedIPsDoesNotOverwriteOtherInboundWhenMismatched is a +// regression test for the same bug class already fixed for Attach +// (addressesFitAmneziaWGInbound), but on the far more common Update path: the +// edit-client form sends one shared AllowedIPs value, and Update's per-inbound +// loop used to broadcast it verbatim to every attached inbound, including one +// it doesn't belong to. A client attached to both wg (10.0.0.5/32) and awg +// (10.8.1.5/32) saving with the wg-labeled value as the single shared field +// must not silently overwrite the awg inbound's own, unrelated address. +func TestUpdateBroadcastAllowedIPsDoesNotOverwriteOtherInboundWhenMismatched(t *testing.T) { + setupBulkDB(t) + inboundSvc := &InboundService{} + svc := &ClientService{} + + wgIb, awgIb, recId := seedDualProtocolClient(t, "dual@x", "10.0.0.5/32", "10.8.1.5/32") + + updated := model.Client{Email: "dual@x", Enable: true, AllowedIPs: []string{"10.0.0.5/32"}} + if _, err := svc.Update(inboundSvc, recId, updated); err != nil { + t.Fatalf("Update: %v", err) + } + + if got := inboundAllowedIPs(t, inboundSvc, wgIb.Id, "dual@x"); len(got) != 1 || got[0] != "10.0.0.5/32" { + t.Fatalf("wg AllowedIPs = %v, want [10.0.0.5/32]", got) + } + if got := inboundAllowedIPs(t, inboundSvc, awgIb.Id, "dual@x"); len(got) != 1 || got[0] != "10.8.1.5/32" { + t.Fatalf("the real bug: awg AllowedIPs = %v, want unchanged [10.8.1.5/32] (must not inherit the wg-labeled shared value)", got) + } +} + +// TestUpdateAllowedIPsByInboundAppliesDistinctValuesPerInbound covers the new +// mechanism the two-field client-edit form uses to intentionally change both +// addresses in one save: distinct, valid, per-inbound override values must +// each land on their own inbound. +func TestUpdateAllowedIPsByInboundAppliesDistinctValuesPerInbound(t *testing.T) { + setupBulkDB(t) + inboundSvc := &InboundService{} + svc := &ClientService{} + + wgIb, awgIb, recId := seedDualProtocolClient(t, "dual@x", "10.0.0.5/32", "10.8.1.5/32") + + updated := model.Client{ + Email: "dual@x", + Enable: true, + AllowedIPsByInbound: map[int][]string{ + wgIb.Id: {"10.0.0.9/32"}, + awgIb.Id: {"10.8.1.9/32"}, + }, + } + if _, err := svc.Update(inboundSvc, recId, updated); err != nil { + t.Fatalf("Update: %v", err) + } + + if got := inboundAllowedIPs(t, inboundSvc, wgIb.Id, "dual@x"); len(got) != 1 || got[0] != "10.0.0.9/32" { + t.Fatalf("wg AllowedIPs = %v, want [10.0.0.9/32]", got) + } + if got := inboundAllowedIPs(t, inboundSvc, awgIb.Id, "dual@x"); len(got) != 1 || got[0] != "10.8.1.9/32" { + t.Fatalf("awg AllowedIPs = %v, want [10.8.1.9/32]", got) + } +} + +// TestCreateSharedAllowedIPsThatDontFitAmneziaWGGetsFreshAllocation is +// Create's counterpart to the Update regression above: adding a brand-new +// client to both wg and awg inbounds at once with a single manually-typed +// address must not hand the awg inbound an address from the wrong subnet -- +// it must fall back to auto-allocating a real, correctly-scoped address +// instead, exactly as if AllowedIPs had been left empty for that inbound. +func TestCreateSharedAllowedIPsThatDontFitAmneziaWGGetsFreshAllocation(t *testing.T) { + setupBulkDB(t) + inboundSvc := &InboundService{} + svc := &ClientService{} + + wgIb := mkInbound(t, 51820, model.WireGuard, wgServerSettings()) + awgIb := mkInbound(t, 443, model.AmneziaWG, amneziawgClientTestSettings) + + payload := &ClientCreatePayload{ + Client: model.Client{Email: "new@x", Enable: true, AllowedIPs: []string{"10.0.0.7/32"}}, + InboundIds: []int{wgIb.Id, awgIb.Id}, + } + if _, err := svc.Create(inboundSvc, payload); err != nil { + t.Fatalf("Create: %v", err) + } + + if got := inboundAllowedIPs(t, inboundSvc, wgIb.Id, "new@x"); len(got) != 1 || got[0] != "10.0.0.7/32" { + t.Fatalf("wg AllowedIPs = %v, want [10.0.0.7/32]", got) + } + got := inboundAllowedIPs(t, inboundSvc, awgIb.Id, "new@x") + if len(got) != 1 { + t.Fatalf("awg AllowedIPs = %v, want exactly one freshly allocated address", got) + } + if got[0] == "10.0.0.7/32" { + t.Fatal("the real bug: awg inbound inherited the wg-shaped shared address instead of allocating its own") + } + if !addressesFitAmneziaWGInbound(got, awgIb) { + t.Fatalf("freshly allocated awg address %v does not actually fit the awg inbound's own subnet", got) + } +} + +// TestCreateAllowedIPsByInboundAppliesDistinctValuesPerInbound is Create's +// counterpart to the Update explicit-override test: the add-client form, +// when attaching to both wg and awg at once with the two-field UI, must be +// able to give each inbound its own manually chosen address in one call. +func TestCreateAllowedIPsByInboundAppliesDistinctValuesPerInbound(t *testing.T) { + setupBulkDB(t) + inboundSvc := &InboundService{} + svc := &ClientService{} + + wgIb := mkInbound(t, 51820, model.WireGuard, wgServerSettings()) + awgIb := mkInbound(t, 443, model.AmneziaWG, amneziawgClientTestSettings) + + payload := &ClientCreatePayload{ + Client: model.Client{ + Email: "new@x", + Enable: true, + AllowedIPsByInbound: map[int][]string{ + wgIb.Id: {"10.0.0.9/32"}, + awgIb.Id: {"10.8.1.9/32"}, + }, + }, + InboundIds: []int{wgIb.Id, awgIb.Id}, + } + if _, err := svc.Create(inboundSvc, payload); err != nil { + t.Fatalf("Create: %v", err) + } + + if got := inboundAllowedIPs(t, inboundSvc, wgIb.Id, "new@x"); len(got) != 1 || got[0] != "10.0.0.9/32" { + t.Fatalf("wg AllowedIPs = %v, want [10.0.0.9/32]", got) + } + if got := inboundAllowedIPs(t, inboundSvc, awgIb.Id, "new@x"); len(got) != 1 || got[0] != "10.8.1.9/32" { + t.Fatalf("awg AllowedIPs = %v, want [10.8.1.9/32]", got) + } +} + +// TestTunnelAllowedIPsByInbound covers the GET-client read side: a two-field +// display needs the real, distinct per-inbound address for each protocol, +// which ClientRecord's own single AllowedIPs column cannot represent. +func TestTunnelAllowedIPsByInbound(t *testing.T) { + setupBulkDB(t) + inboundSvc := &InboundService{} + svc := &ClientService{} + + wgIb, awgIb, _ := seedDualProtocolClient(t, "dual@x", "10.0.0.5/32", "10.8.1.5/32") + vlessIb := mkInbound(t, 8443, model.VLESS, clientsSettings(t, nil)) + + got, err := svc.TunnelAllowedIPsByInbound(inboundSvc, "dual@x", []int{wgIb.Id, awgIb.Id, vlessIb.Id, 999999}) + if err != nil { + t.Fatalf("TunnelAllowedIPsByInbound: %v", err) + } + if len(got) != 2 { + t.Fatalf("result = %v, want exactly 2 entries (vless and the nonexistent id must be skipped)", got) + } + if got[wgIb.Id] != "10.0.0.5/32" { + t.Fatalf("wg entry = %q, want 10.0.0.5/32", got[wgIb.Id]) + } + if got[awgIb.Id] != "10.8.1.5/32" { + t.Fatalf("awg entry = %q, want 10.8.1.5/32", got[awgIb.Id]) + } + if _, ok := got[vlessIb.Id]; ok { + t.Fatalf("a non-tunnel (VLESS) inbound must not appear in the result") + } + if _, ok := got[999999]; ok { + t.Fatalf("a nonexistent inbound id must not appear in the result") + } +}