fix(amneziawg): refuse a row's own relay port and keep a disabled row's slot reserved (#6544)

* fix(amneziawg): refuse a WireGuard port that is the row's own relay port

All three relay checks filter themselves out of the candidates with id !=
ignoreId, so nothing ever compared an AmneziaWG row's own WireGuard listen port
with the relay port its own id derives. Saving a row on that exact port left the
embedded device (UDP on the inbound's listen address, amneziawgnet/device.go:137)
and its injected relay (TCP and UDP on 127.0.0.1, amneziawgnet/relay.go:47-61)
bound to the same UDP port, so whichever loses the race dies -- and when the
relay loses it, Xray refuses the whole config and takes every other protocol on
the host with it. The first AmneziaWG inbound on port 65101 was enough to reach
it: id 1 derives exactly that port.

The row now states the rule its three siblings do: it owns the slot its id
derives. A node-hosted row still keeps its own port, since it binds no relay on
this host.

TestAddInbound_AmneziawgRefusesItsOwnRelayPort and
TestUpdateInbound_AmneziawgRefusesItsOwnRelayPort fail without this -- both were
watched red first -- and pin the two separate call sites, AddInbound's post-Save
block and checkPortConflictTx's ignoreId > 0 block.

* fix(amneziawg): keep a disabled row's relay port reserved for port forwards

loadPortConflictContext filtered its query with enable = true, so a client's
ForwardedPorts spec could claim the relay port a disabled AmneziaWG row's id
derives. That row's relay appears with its first client -- a path that runs no
port check -- and when the relay then loses the loopback bind race to the
forward listener, Xray refuses the whole config instead of losing one forward
(#6542 review, arrived with #6540).

The context now loads every local row and gates only the ordinary-port compare on
enable, which is what a disabled row's own port is worth: free. Its relay slot is
not free, which is the rule #6540 already states for the other two guards.

TestCheckForwardedPortsConflict_DisabledAmneziawgRelayPortIsReserved fails
without this -- watched red first -- and passes with it, while
TestCheckForwardedPortsConflict_IgnoresDisabledInboundPort keeps proving that a
disabled inbound's own port stays available.

* fix(amneziawg): re-run the forward guard once a new row has its own ports

normalizeAmneziaWGSettings validates every client's ForwardedPorts before the row
is saved, and loadPortConflictContext then reads the database -- so the new
AmneziaWG row is never a candidate for itself. A client could forward exactly the
relay port the row's own id derives, or its own WireGuard listen port, and the
create was accepted: at runtime the panel's wildcard forward listener and Xray's
127.0.0.1 relay race for the same port, and a lost relay bind makes Xray refuse
the whole generated config (#6544 review, pre-existing).

The post-Save block is the only place the id is known, so it re-runs the guard
there. Both callers now share amneziaWGForwardedPortsConflict, so the collision
message lives in one place instead of two.

TestAddInbound_AmneziawgRefusesAClientForwardingItsOwnRelayPort fails without
this -- watched red first -- and passes with it.

* fix(amneziawg): stop blocking stored forward specs on a disabled row's slot

Round 2 flagged this PR's widening as the one MEDIUM it introduced, and the code
confirms it: UpdateInboundClient carries a stored ForwardedPorts spec forward for
a partial edit (client_inbound_apply.go:763-765) and re-validates it (:772 and
:909), so after an in-place upgrade an edit that never submitted the field -- a
bot enable/expiry toggle -- is refused over a slot the operator did not touch,
for a relay injectAmneziawgnetSocks does not emit while the row is disabled. The
inbound-save path re-validates every stored spec the same way.

The trade does not pay for itself: the slot this reserves is claimable only by a
spec an operator authors onto 65101-65535, while the cost lands on unrelated
operations. The precise fix -- refuse a newly claimed spec rather than a stored
one, and check the enable transition in SetInboundEnable, where the conflict is
actually created -- is larger than the hole, so the slot goes back to a
documented pre-existing item with its own follow-up.

The create-path re-run added in 80eb5712 is unaffected: it reads the settings
submitted in the same request, so it never refuses a stored value, and its test
still passes.

internal/web/service/inbound.go

@@ -1238,8 +1238,11 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo
 			return err
 		}
 		// The relay port is derived from the id, only known after Save, and only a
-		// local row owns one: checkPortConflictTx ran neither check with ignoreId==0.
+		// local row owns one: checkPortConflictTx ran no relay check with ignoreId==0.
 		if inbound.NodeID == nil && inbound.Protocol == model.AmneziaWG {
+			if self := amneziawgnetSocksSelfConflict(inbound, inbound.Id); self != "" {
+				return common.NewError(self)
+			}
 			conflict, cErr := checkAmneziawgnetSocksRelayCollision(tx, inbound.Id)
 			if cErr != nil {
 				return cErr
@@ -1254,6 +1257,11 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo
 			if conflict != nil {
 				return common.NewError(conflict.String())
 			}
+			// The clients' forward specs were validated while this row had no id,
+			// so the ports it now derives were never in the guard's context.
+			if aErr := s.checkAmneziaWGForwardedPorts(tx, inbound.Settings); aErr != nil {
+				return aErr
+			}
 		}
 		// Emails seeded here (import's ClientStats, e.g. the controller's forced
 		// Enable=true on every imported stat row) are authoritative for this call

internal/web/service/inbound_amneziawg.go

@@ -278,8 +278,8 @@ func (s *InboundService) normalizeAmneziaWGSettings(inbound *model.Inbound, oldS
 	}
 	for i := range parsed.Clients {
 		c := &parsed.Clients[i]
-		if hit := s.checkForwardedPortsConflict(portCtx, c.ForwardedPorts); hit != "" {
-			return fmt.Errorf("amneziawg: client %q forwardedPorts collides with %s", c.Email, hit)
+		if err := s.amneziaWGForwardedPortsConflict(portCtx, c); err != nil {
+			return err
 		}
 		if err := amneziawg.ValidateConfigValue("email", c.Email); err != nil {
 			return fmt.Errorf("amneziawg: %w", err)
@@ -313,21 +313,15 @@ func (s *InboundService) normalizeAmneziaWGSettings(inbound *model.Inbound, oldS
 	return nil
 }
 
-// portConflictContext caches the state checkForwardedPortsConflict needs —
-// the panel's own port and this host's enabled inbound ports — so validating
-// N clients in one save (normalizeAmneziaWGSettings, or a bulk client add)
-// costs one query total instead of N. Load it once with
-// loadPortConflictContext and pass it to every checkForwardedPortsConflict
-// call in that batch.
+// portConflictContext caches what checkForwardedPortsConflict needs — the panel's
+// own port and this host's enabled rows — so one save costs one query, not N.
 type portConflictContext struct {
 	webPort  int
 	inbounds []*model.Inbound
 }
 
-// loadPortConflictContext loads the panel's own port and every enabled
-// inbound hosted on THIS panel (node_id IS NULL) — an inbound hosted on a
-// different node listens on that node's own host, never this one, so it can
-// never collide with a DNAT rule this process installs.
+// loadPortConflictContext loads the panel's own port and every enabled inbound
+// hosted on THIS panel: a node-hosted one listens on that node's host, not here.
 func (s *InboundService) loadPortConflictContext(db *gorm.DB) (portConflictContext, error) {
 	var ctx portConflictContext
 	if webPort, err := (&SettingService{}).GetPort(); err == nil {
@@ -339,15 +333,37 @@ func (s *InboundService) loadPortConflictContext(db *gorm.DB) (portConflictConte
 	return ctx, err
 }
 
-// checkForwardedPortsConflict reports whether a client's ForwardedPorts spec
-// exceeds the cap, covers the panel's own web port, one of this host's own
-// enabled inbound listen ports, or an AmneziaWG inbound's own phantom SOCKS5
-// relay port (SOCKSPortForInbound -- never a real inbounds row, so the loop
-// below can't see it any other way). A collision on the SOCKS5 port would
-// let a port-forward listener race Xray's own relay for the bind and, if it
-// wins, take down that inbound's entire relay rather than just one forward.
-// Returns a human-readable description of the first collision found, or ""
-// when there is none.
+// amneziaWGForwardedPortsConflict renders one client's ForwardedPorts collision,
+// or nil: the single copy both the pre-Save pass and the post-Save re-run use.
+func (s *InboundService) amneziaWGForwardedPortsConflict(ctx portConflictContext, c *model.Client) error {
+	hit := s.checkForwardedPortsConflict(ctx, c.ForwardedPorts)
+	if hit == "" {
+		return nil
+	}
+	return fmt.Errorf("amneziawg: client %q forwardedPorts collides with %s", c.Email, hit)
+}
+
+// checkAmneziaWGForwardedPorts re-runs the guard over one row's stored clients:
+// on create it ran before Save, when the row's own ports were not in the context.
+func (s *InboundService) checkAmneziaWGForwardedPorts(db *gorm.DB, settings string) error {
+	var parsed amneziawg.InboundSettings
+	if err := json.Unmarshal([]byte(settings), &parsed); err != nil {
+		return nil
+	}
+	ctx, err := s.loadPortConflictContext(db)
+	if err != nil {
+		return err
+	}
+	for i := range parsed.Clients {
+		if err := s.amneziaWGForwardedPortsConflict(ctx, &parsed.Clients[i]); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// checkForwardedPortsConflict names the panel, inbound or AmneziaWG relay port a
+// client's ForwardedPorts spec would collide with: a lost bind race kills the relay.
 func (s *InboundService) checkForwardedPortsConflict(ctx portConflictContext, forwardedPorts string) string {
 	if forwardedPorts == "" {
 		return ""

internal/web/service/inbound_amneziawg_relay_window_test.go

@@ -24,6 +24,14 @@ func awgRelayWindowSettings(t *testing.T, tag string) string {
 		clientPub + `","allowedIPs":["10.8.1.2/32"]}]}`
 }
 
+// awgRelayWindowSettingsWithForward is awgRelayWindowSettings with one client's
+// forwardedPorts set, the field the create-time guard validates.
+func awgRelayWindowSettingsWithForward(t *testing.T, tag, forwardedPorts string) string {
+	t.Helper()
+	settings := awgRelayWindowSettings(t, tag)
+	return strings.Replace(settings, `"enable":true`, `"enable":true,"forwardedPorts":"`+forwardedPorts+`"`, 1)
+}
+
 // pushInboundIDSequence makes the next inbounds insert land on nextID, standing
 // in for a long-lived database whose AUTOINCREMENT counter has climbed there.
 func pushInboundIDSequence(t *testing.T, nextID int) {
@@ -168,6 +176,80 @@ func TestCheckPortConflict_DisabledAmneziawgStillOwnsItsRelaySlot(t *testing.T)
 	}
 }
 
+// The forwarded-ports guard runs before Save, when the row has no id yet, so a
+// client's spec never saw the relay port the row itself derives.
+func TestAddInbound_AmneziawgRefusesAClientForwardingItsOwnRelayPort(t *testing.T) {
+	setupConflictDB(t)
+
+	placeholder := addAmneziaWGInbound(t, "awg-placeholder", 51820, true)
+	ownPort := amneziawgnet.SOCKSPortForInbound(placeholder.Id + 1)
+
+	_, _, err := (&InboundService{}).AddInbound(&model.Inbound{
+		Tag:      "awg-forward",
+		Enable:   true,
+		Listen:   "0.0.0.0",
+		Port:     51821,
+		Protocol: model.AmneziaWG,
+		Settings: awgRelayWindowSettingsWithForward(t, "awg-forward", fmt.Sprintf("%d", ownPort)),
+	})
+	if err == nil {
+		t.Fatalf("inbound #%d derives relay port %d and its own client forwards that port; the create must be refused",
+			placeholder.Id+1, ownPort)
+	}
+	if !strings.Contains(err.Error(), "forwardedPorts") {
+		t.Fatalf("the refusal must come from the forwarded-ports guard, got %v", err)
+	}
+}
+
+// The row's own WireGuard port can be the relay port its own id derives, and
+// every relay check excludes that id, so nothing else compares the two.
+func TestAddInbound_AmneziawgRefusesItsOwnRelayPort(t *testing.T) {
+	setupConflictDB(t)
+
+	// Read the sequence instead of assuming id 1: the victim's own derived port
+	// has to be known before it is created.
+	placeholder := addAmneziaWGInbound(t, "awg-placeholder", 51820, true)
+	selfPort := amneziawgnet.SOCKSPortForInbound(placeholder.Id + 1)
+
+	_, _, err := (&InboundService{}).AddInbound(&model.Inbound{
+		Tag:      "awg-self",
+		Enable:   true,
+		Listen:   "0.0.0.0",
+		Port:     selfPort,
+		Protocol: model.AmneziaWG,
+		Settings: awgRelayWindowSettings(t, "awg-self"),
+	})
+	if err == nil {
+		t.Fatalf("WireGuard port %d is inbound #%d's own relay port; the create must be refused",
+			selfPort, placeholder.Id+1)
+	}
+	if !strings.Contains(err.Error(), "relay port") {
+		t.Fatalf("the refusal must say the port is an automatic relay one, got %v", err)
+	}
+}
+
+// The edit path knows the id the relay port comes from, so it has to refuse the
+// same self-collision -- the reverse check skips the row it computes for.
+func TestUpdateInbound_AmneziawgRefusesItsOwnRelayPort(t *testing.T) {
+	setupConflictDB(t)
+	created := addAmneziaWGInbound(t, "awg-self-edit", 51820, true)
+
+	edit := *created
+	edit.Port = amneziawgnet.SOCKSPortForInbound(created.Id)
+	if edit.Port == created.Port {
+		t.Fatalf("fixture: inbound #%d already listens on its derived relay port", created.Id)
+	}
+
+	_, _, err := (&InboundService{}).UpdateInbound(&edit)
+	if err == nil {
+		t.Fatalf("WireGuard port %d is inbound #%d's own relay port; the save must be refused",
+			edit.Port, created.Id)
+	}
+	if !strings.Contains(err.Error(), "relay port") {
+		t.Fatalf("the refusal must say the port is an automatic relay one, got %v", err)
+	}
+}
+
 // A row adopted from a node keeps the protocol it arrived with and its central
 // id (inbound_node.go:737), but gets no relay -- so its slot can never be taken.
 func TestCheckPortConflict_NodeAssignedAmneziawgOwnsNoRelaySlot(t *testing.T) {
@@ -197,4 +279,16 @@ func TestCheckPortConflict_NodeAssignedAmneziawgOwnsNoRelaySlot(t *testing.T) {
 		t.Fatalf("id %d is node-assigned and binds no relay, so it cannot collide; got %q",
 			collidingID, got.String())
 	}
+
+	// The same rule covers the row's own port: with no relay on this host, its
+	// WireGuard port may legitimately BE the port its id would derive.
+	adopted.Port = amneziawgnet.SOCKSPortForInbound(collidingID)
+	got, err = (&InboundService{}).checkPortConflict(adopted, collidingID)
+	if err != nil {
+		t.Fatalf("checkPortConflict: %v", err)
+	}
+	if got != nil {
+		t.Fatalf("id %d is node-assigned and binds no relay, so its own port is not a conflict; got %q",
+			collidingID, got.String())
+	}
 }

internal/web/service/port_conflict.go

@@ -223,6 +223,9 @@ func checkPortConflictTx(db *gorm.DB, inbound *model.Inbound, ignoreId int) (*po
 	// The reverse direction, only meaningful once the id is known -- AddInbound
 	// runs it after Save. Only a local row owns a relay slot (#6537 review).
 	if inbound.NodeID == nil && inbound.Protocol == model.AmneziaWG && ignoreId > 0 {
+		if self := amneziawgnetSocksSelfConflict(inbound, ignoreId); self != "" {
+			return nil, common.NewError(self)
+		}
 		conflict, err := checkAmneziawgnetSocksRelayCollision(db, ignoreId)
 		if err != nil {
 			return nil, err
@@ -330,6 +333,20 @@ func checkAmneziawgnetSocksRelayCollision(db *gorm.DB, id int) (*portConflictDet
 	return nil, nil
 }
 
+// amneziawgnetSocksSelfConflict: a row's own WireGuard port vs the relay port its
+// own id derives -- all three checks below exclude that id, so nothing else does.
+func amneziawgnetSocksSelfConflict(inbound *model.Inbound, id int) string {
+	if id <= 0 || inbound.NodeID != nil || !listenOverlaps("127.0.0.1", inbound.Listen) {
+		return ""
+	}
+	relayPort := amneziawgnet.SOCKSPortForInbound(id)
+	if inbound.Port != relayPort {
+		return ""
+	}
+	return fmt.Sprintf("WireGuard port %d is inbound #%d's own SOCKS5 relay port on 127.0.0.1; choose a different WireGuard port",
+		relayPort, id)
+}
+
 // checkAmneziawgnetSocksReverseConflict mirrors checkAmneziawgnetSocksConflict:
 // does id's own derived relay port collide with some other inbound's port.
 func checkAmneziawgnetSocksReverseConflict(db *gorm.DB, id int) (*portConflictDetail, error) {