Merge branch 'main' into main

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) {