fix: preserve per-inbound WireGuard peer addresses - #6344
Conversation
Clients are stored once per email in the client table, so when the same email exists on more than one WireGuard inbound the shared record's AllowedIPs and PreSharedKey win for every inbound. A client present on both a WG and an AWG tunnel was emitted with one tunnel's address on both, so the second tunnel's peer got the wrong allowedIPs. Read the per-inbound client settings for WireGuard inbounds and, when the inbound carries its own entry for that email, use its AllowedIPs and PreSharedKey when building the peer.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if inboundClient, ok := wireguardClientsByEmail[strings.ToLower(strings.TrimSpace(c.Email))]; ok { | ||
| c.AllowedIPs = inboundClient.AllowedIPs | ||
| c.PreSharedKey = inboundClient.PreSharedKey | ||
| } |
There was a problem hiding this comment.
🔴 Important — the emitted peer now disagrees with the config the subscription server hands the client.
This override moves the server peer's allowedIPs/preSharedKey to the inbound's settings JSON, but the client-facing half of the same pair still comes from the shared clients row:
matchingClientsprimes the per-request cache fromGetClientsBySubId→ListForInboundBySubId, a plainTable("clients")join —Lines 261 to 278 in b455706
clientForLinkreturns that primed row and never reaches the settings-JSON fallback below it —Lines 142 to 157 in b455706
genWireguardLinkemitsaddressandpresharedkeystraight off it —(Lines 666 to 680 in b455706
json_service.goandclash_service.goreceive the same client object)
Before this change both sides read that one row, so they matched by construction. The override only does anything when the row and the settings entry differ — i.e. exactly the #6328 topology — so every behaviour change this PR makes desynchronizes the subscription-issued config.
Concretely, dual@x on WG inbound A (10.0.0.5/32, wg-psk) and AWG inbound B (10.8.1.5/32, awg-psk), B synced last so the shared row holds B's values:
- before: peer and subscription
.confboth say10.8.1.5/32+awg-psk→ tunnel connects, with the duplicated address [Bug]: AmneziaWG together with WireGuard for the same client invokes WG doesn't work #6328 reports. - after: peer says
10.0.0.5/32+wg-psk, subscription still hands out10.8.1.5/32+awg-psk→ PSK mismatch and a source address outside the peer'sallowedIPsfilter, so a tunnel that worked stops working.
To be fair to the change: the panel's own link export goes the other way and is improved by it — LinksForClient builds a fresh SubService and calls GetLink with an unprimed cache (
Lines 37 to 42 in b455706
clientForLink falls through to GetClients(inbound) and already read the settings JSON. So this is a channel swap, not a pure loss. But subscription is the channel most deployments distribute through, so it is the wrong half to break.
Closing it properly means resolving AllowedIPs/PreSharedKey per inbound on the subscription path too — e.g. having matchingClients/clientForLink overlay the inbound's own settings entry for wireguard/amneziawg clients — so all three subscription formats agree with the peer this function emits.
| c.AllowedIPs = inboundClient.AllowedIPs | ||
| c.PreSharedKey = inboundClient.PreSharedKey | ||
| } | ||
| wgPeers = append(wgPeers, model.WireguardPeerFromClient(c)) |
There was a problem hiding this comment.
🟣 Pre-existing — this predates the change (nothing outside xray.go moved), so it is not a reason to hold the PR, but it sits inside the block just added and is the same #6328 failure in a harsher form.
WireguardPeerFromClient builds the peer from four fields — PublicKey, AllowedIPs, PreSharedKey, KeepAlive —
3x-ui/internal/database/model/model.go
Lines 432 to 448 in b455706
All four come off the same shared per-email row (ClientRecord.Email is uniqueIndex), and the override above restores two of them. c.PublicKey — the field that actually identifies the peer to xray — is still whichever inbound wrote the row last, and a wrong publicKey means no handshake at all rather than a mis-routed one.
It is reachable through the panel, not just the API: ClientBulkAddModal sends no privateKey/publicKey while allowing several inboundIds, BulkCreate calls AddInboundClient once per inbound with that key-less client, each WG/AWG inbound then mints its own keypair (
3x-ui/internal/web/service/client_wireguard.go
Lines 204 to 212 in b455706
client_amneziawg.go), and applyClientRecordMerge keeps the last non-empty one — 3x-ui/internal/web/service/client_link.go
Lines 47 to 55 in b455706
Extending the override to PublicKey (and KeepAlive) would close it in the same two lines, though only once the subscription-path divergence flagged above is resolved — otherwise it widens that mismatch to the key material as well.
| func TestGetXrayConfigWireGuardDisabledDualProtocolClientExcluded(t *testing.T) { | ||
| seedDualTunnelClient(t, false) | ||
|
|
||
| peers := wgPeerList(t, wgInboundEmittedSettings(t, "wg-dual")) | ||
| if len(peers) != 0 { | ||
| t.Fatalf("expected disabled dual-protocol client to be excluded, got %v", peers) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Nit — this test passes with and without the fix, which CLAUDE.md rejects outright:
A test must fail without its fix. Write it, revert the fix, watch it go red, restore. A test that passes either way is worse than no test: it certifies nothing and then gets cited as proof the fix works.
seedDualTunnelClient(t, false) syncs the AWG inbound last with Enable: false, and applyClientRecordMerge assigns row.Enable = incoming.Enable unconditionally (
3x-ui/internal/web/service/client_link.go
Lines 54 to 60 in b455706
GetXrayConfig then drops the client at if !c.Enable { continue } — 3x-ui/internal/web/service/xray.go
Lines 211 to 214 in b455706
switch ever reaches case model.WireGuard:. The new override never touches Enable or enableMap, so len(peers) == 0 holds identically with the xray.go hunk reverted.
It is also redundant with the pre-existing TestGetXrayConfigWireGuardDisabledClientExcluded, which already pins that same guard without the dual-protocol setup. Worth deleting, or repurposing to assert something the override could actually break.
(TestGetXrayConfigWireGuardUsesInboundLocalTunnelFields above it is sound — it does go red on revert, as the description claims.)
Code review1 🔴 / 1 🟡 / 1 🟣 Reviewed head:
Coverage
|
GetXrayConfig built WireGuard peers from ListForInbound, which returns the shared clients row. That row has a single wg_allowed_ips / wg_pre_shared_key column, so a client attached to two WireGuard inbounds — a case this fork supports through AllowedIPsByInbound and shows per inbound via TunnelAllowedIPsByInbound — had both peers emitted with whichever address was written last. The per-inbound value lives in each inbound's own settings JSON, which is also what UpdateInbound feeds back into SyncInbound, so read the peer's address and preshared key from there. The live gRPC AddInbound path was already correct: it converts from the settings JSON via model.WireguardClientsToPeers. Only the full-config path was wrong, which made the symptom "works after saving, breaks after a restart". Same bug as upstream MHSanaei#6344, found by reviewing it against our tree; the mechanics differ because our clients are normalized. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary
WireGuard peers were built from the shared per-email client record, so a client present on both a WireGuard and an AmneziaWG inbound got one tunnel's
allowedIPsandpreSharedKeyon both. This reads the per-inbound client settings and uses the inbound's own entry when one exists for that email.Why
Closes #6328
Clients are stored once per email in the client table.
GetXrayConfigbuilt each WireGuard peer from that shared record, so the per-inbound values were lost whenever the same email appeared on two WireGuard-family inbounds. The second tunnel's peer was then emitted with the first tunnel's address, which breaks routing for that client.The inbound already carries its own client settings; they were simply not consulted on the WireGuard path.
Type of change
Areas affected
How was this tested?
Added
TestGetXrayConfigWireGuardUsesInboundLocalTunnelFields, which seeds one email on both a WG inbound (10.0.0.5/32,wg-psk) and an AWG inbound (10.8.1.5/32,awg-psk) and asserts each inbound's peer keeps its own address and pre-shared key.The test pins the bug rather than merely covering the path. Reverting only
internal/web/service/xray.goand keeping the test:That is the reported symptom exactly: the AWG tunnel's address on the WG peer.
Not tested: no run against a live Xray instance with two active tunnels. The assertion is on generated config, not on observed traffic.
Screenshots / recordings
N/A. Backend config generation, no UI change.
Breaking changes
None. Behaviour only changes where an inbound carries its own client entry for that email; when it does not, the existing shared-record values are used exactly as before.
Checklist
go build ./...and the test suite pass locally.npm run lint,npm run typecheck, andnpm run buildpass.Frontend checks are unchecked because there are no frontend changes. Docs are unchecked because the corrected behaviour is what the panel already documents; nothing user-facing changed.
AI assistance disclosure
internal/web/service/xray.goandinternal/web/service/xray_wireguard_config_test.goonly.AI was used for assistance.