fix(ports): refuse an inbound on a port an AmneziaWG peer forwards (#6554)

* fix(ports): refuse an inbound on a port an AmneziaWG peer forwards checkForwardedPortsConflict only ever ran from the AmneziaWG save path, and only in one direction: an AmneziaWG client's forwardedPorts were checked against the ports other inbounds already hold, while the reverse -- an ordinary inbound saved onto a port some peer forwards -- had no guard at all. The forward listener binds that port on every interface in both directions (amneziawgnet/portfwd.go's attachTCP/attachUDP), so the two listeners want the same socket: the loser either leaves the peer's forward silently dead or fails the inbound's listen. checkPortConflictTx now resolves that owner the same way the relay-slot checks do -- same host, peers derived from the stored settings with the shared InstanceFromInbound -- and names the peer in the refusal. Sitting inside checkPortConflictTx covers both the save and the enable path added in #6549. TestAddInboundRefusesAPortAnAmneziaWGPeerForwards fails without this -- watched red, the create is allowed -- and its node-row companion pins the scoping that keeps a node row legal on a locally forwarded port. * fix(ports): name only a peer that binds as the owner of a forwarded port The owner lookup read instance.Peers and ForwardedPortsInclude directly, so a peer the forward supervisor skips (no email, or no address the tunnel routes to) was reported as holding a port nothing binds -- refusing a create that is legal with a message naming a row whose own port is its WireGuard one. It also repeated the candidate's listen address as the forward's location, though the forward binds :port on every interface. Share the supervisor's own gate through amneziawgnet.ForwardedPortOwner, report the wildcard bind, and propagate a failed owner query instead of reading it as "no conflict", matching the sibling checks in the same file. * style(ports): keep the forwarded-key doc block within the 2-line cap The reworded desiredPortForwardKeys doc ran to three lines, against the rule this repo sets for committed Go comments.
2026-09-16 15:17:14 +00:00 · 2026-09-15 16:58:21 +03:00
parent d9c7c76fb0
commit ac3fc12077
3 changed files with 220 additions and 9 deletions
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"strings"

+	"github.com/mhsanaei/3x-ui/v3/internal/amneziawg"
 	"github.com/mhsanaei/3x-ui/v3/internal/amneziawgnet"
 	"github.com/mhsanaei/3x-ui/v3/internal/database"
 	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
@@ -108,8 +109,11 @@ type portConflictDetail struct {
 	Listen    string
 	Port      int
 	// Relay marks Port as an automatic loopback relay port, not a configured one.
-	Relay      bool
-	Transports transportBits
+	Relay bool
+	// ForwardedBy is the peer whose port forward holds Port, when that is the
+	// reason for the conflict.
+	ForwardedBy string
+	Transports  transportBits
 }

 // String renders the detail as a single-line, user-facing summary.
@@ -134,6 +138,10 @@ func (d *portConflictDetail) String() string {
 	if d.Relay {
 		port = fmt.Sprintf("relay port %d", d.Port)
 	}
+	if d.ForwardedBy != "" {
+		return fmt.Sprintf("%s (%s) already forwarded on inbound %s on %s by its client %s",
+			port, transportTagSuffix(d.Transports), name, listen, d.ForwardedBy)
+	}
 	return fmt.Sprintf("%s (%s) already used by inbound %s on %s",
 		port, transportTagSuffix(d.Transports), name, listen)
 }
@@ -242,6 +250,17 @@ func checkPortConflictTx(db *gorm.DB, inbound *model.Inbound, ignoreId int) (*po
 		}
 	}

+	// A forwarded port is not a column and not a relay slot, so the query below
+	// cannot see it either: the port is bound by the peer's forward listener.
+	forwardedBy, err := amneziawgForwardedPortOwner(db, inbound, ignoreId)
+	if err != nil {
+		return nil, err
+	}
+	if forwardedBy != nil {
+		forwardedBy.Transports = newBits
+		return forwardedBy, nil
+	}
+
 	var candidates []*model.Inbound
 	q := db.Model(model.Inbound{}).Where("port = ?", inbound.Port)
 	if ignoreId > 0 {
@@ -275,6 +294,43 @@ func checkPortConflictTx(db *gorm.DB, inbound *model.Inbound, ignoreId int) (*po
 	return nil, nil
 }

+// amneziawgForwardedPortOwner names the AmneziaWG row on the same host whose peer
+// forwards inbound's port -- a bind on every interface that only the AWG side checked.
+func amneziawgForwardedPortOwner(db *gorm.DB, inbound *model.Inbound, ignoreId int) (*portConflictDetail, error) {
+	var rows []*model.Inbound
+	q := db.Model(model.Inbound{}).Where("protocol = ?", model.AmneziaWG)
+	if ignoreId > 0 {
+		q = q.Where("id != ?", ignoreId)
+	}
+	if err := q.Find(&rows).Error; err != nil {
+		return nil, err
+	}
+	for _, row := range rows {
+		if !sameNode(row.NodeID, inbound.NodeID) {
+			continue
+		}
+		instance, ok := amneziawg.InstanceFromInbound(row)
+		if !ok {
+			continue
+		}
+		email, forwards := amneziawgnet.ForwardedPortOwner(instance, inbound.Port)
+		if !forwards {
+			continue
+		}
+		return &portConflictDetail{
+			InboundID: row.Id,
+			Remark:    row.Remark,
+			Tag:       row.Tag,
+			// the forward binds :port on every interface, wherever the
+			// candidate asked to listen.
+			Listen:      "",
+			Port:        inbound.Port,
+			ForwardedBy: email,
+		}, nil
+	}
+	return nil, nil
+}
+
 // checkAmneziawgnetSocksConflict: inbound's port vs the relay port every matching
 // local row reserves, emitted or not; db keeps it in the caller's transaction (#6225).
 func checkAmneziawgnetSocksConflict(db *gorm.DB, inbound *model.Inbound, ignoreId int, newBits transportBits) (*portConflictDetail, error) {
@@ -0,0 +1,135 @@
+package service
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/mhsanaei/3x-ui/v3/internal/database"
+	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
+)
+
+// checkForwardedPortsConflict only ran from the AmneziaWG save path, so an
+// ordinary inbound could take a port a peer forwards on every interface.
+func TestAddInboundRefusesAPortAnAmneziaWGPeerForwards(t *testing.T) {
+	const forwarded = 8443
+	cases := []struct {
+		name    string
+		port    int
+		wantErr bool
+	}{
+		{"the forwarded port", forwarded, true},
+		{"a free port", forwarded + 1, false},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			setupConflictDB(t)
+			seedInboundConflict(t, "awg-forward", "0.0.0.0", 51820, model.AmneziaWG, ``,
+				awgRelayWindowSettingsWithForward(t, "awg-forward", "8443"))
+
+			_, _, err := (&InboundService{}).AddInbound(&model.Inbound{
+				Tag: "user-inbound", Enable: true, Listen: "0.0.0.0", Port: tc.port,
+				Protocol: model.VLESS, StreamSettings: `{"network":"tcp"}`, Settings: `{"clients":[]}`,
+			})
+			if !tc.wantErr {
+				if err != nil {
+					t.Fatalf("port %d is free; the create must be allowed: %v", tc.port, err)
+				}
+				return
+			}
+			if err == nil {
+				t.Fatalf("port %d is forwarded by a peer of another inbound; the create must be refused", tc.port)
+			}
+			if !strings.Contains(err.Error(), "awg-forward@relay-window") {
+				t.Fatalf("the refusal must name the peer holding the port, got %v", err)
+			}
+		})
+	}
+}
+
+// A peer the forward supervisor opens no listener for holds no port: it has no
+// email, or no address the tunnel can route to, and Reconcile skips it either way.
+func TestAddInboundAllowsAPortNoPeerCanActuallyForward(t *testing.T) {
+	cases := []struct {
+		name     string
+		settings func(t *testing.T) string
+	}{
+		{
+			name: "a peer with no email",
+			settings: func(t *testing.T) string {
+				t.Helper()
+				return replaceFirst(t, awgRelayWindowSettingsWithForward(t, "awg-forward", "8443"),
+					`"email":"awg-forward@relay-window"`, `"email":""`)
+			},
+		},
+		{
+			name: "an IPv6-only peer on a row without IPv6",
+			settings: func(t *testing.T) string {
+				t.Helper()
+				return replaceFirst(t, awgRelayWindowSettingsWithForward(t, "awg-forward", "8443"),
+					`"allowedIPs":["10.8.1.2/32"]`, `"allowedIPs":["fd00::2/128"]`)
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			setupConflictDB(t)
+			seedInboundConflict(t, "awg-forward", "0.0.0.0", 51820, model.AmneziaWG, ``, tc.settings(t))
+
+			if _, _, err := (&InboundService{}).AddInbound(&model.Inbound{
+				Tag: "user-inbound", Enable: true, Listen: "0.0.0.0", Port: 8443,
+				Protocol: model.VLESS, StreamSettings: `{"network":"tcp"}`, Settings: `{"clients":[]}`,
+			}); err != nil {
+				t.Fatalf("nothing binds 8443 for this peer; the create must be allowed: %v", err)
+			}
+		})
+	}
+}
+
+// The refusal has to point at where the socket really is: the forward listens on
+// every interface, so repeating the candidate's requested address asserts a lie.
+func TestForwardedPortRefusalNamesTheWildcardBind(t *testing.T) {
+	setupConflictDB(t)
+	seedInboundConflict(t, "awg-forward", "0.0.0.0", 51820, model.AmneziaWG, ``,
+		awgRelayWindowSettingsWithForward(t, "awg-forward", "8443"))
+
+	_, _, err := (&InboundService{}).AddInbound(&model.Inbound{
+		Tag: "user-inbound", Enable: true, Listen: "10.0.0.5", Port: 8443,
+		Protocol: model.VLESS, StreamSettings: `{"network":"tcp"}`, Settings: `{"clients":[]}`,
+	})
+	if err == nil {
+		t.Fatal("the port is forwarded on every interface, including 10.0.0.5; the create must be refused")
+	}
+	if !strings.Contains(err.Error(), " on * by its client ") {
+		t.Fatalf("the refusal must place the forward on every interface, got %v", err)
+	}
+}
+
+func replaceFirst(t *testing.T, s, old, new string) string {
+	t.Helper()
+	if !strings.Contains(s, old) {
+		t.Fatalf("fixture no longer contains %s", old)
+	}
+	return strings.Replace(s, old, new, 1)
+}
+
+// The forward listener runs where the AmneziaWG row runs, so a node row sharing
+// a local peer's port stays legal -- the scoping every other guard here uses.
+func TestAddInboundAllowsANodeRowOnALocallyForwardedPort(t *testing.T) {
+	setupConflictDB(t)
+	seedInboundConflict(t, "awg-forward", "0.0.0.0", 51820, model.AmneziaWG, ``,
+		awgRelayWindowSettingsWithForward(t, "awg-forward", "8443"))
+
+	node := &model.Node{Name: "n1", Address: "127.0.0.1", Port: 2096, Scheme: "https", Enable: true, Status: "online"}
+	if err := database.GetDB().Create(node).Error; err != nil {
+		t.Fatalf("seed node: %v", err)
+	}
+	if _, _, err := (&InboundService{}).AddInbound(&model.Inbound{
+		Tag: "node-inbound", Enable: true, Listen: "0.0.0.0", Port: 8443,
+		Protocol: model.VLESS, StreamSettings: `{"network":"tcp"}`, Settings: `{"clients":[]}`,
+		NodeID: &node.Id,
+	}); err != nil {
+		t.Fatalf("a node row does not bind here; the create must be allowed: %v", err)
+	}
+}