fix(amneziawg): let an emptied forwardedPorts withdraw a client's forwards

UpdateInboundClient carried the stored forwardedPorts forward whenever
the incoming value was empty, and Client.ForwardedPorts was a plain
omitempty string, so clearing the field in the client editor, as its
hint says to, reported success while the settings kept the old ports
and the host kept relaying them to the client's device.

The field is now a pointer, as KeepAlive is: nil (not sent) keeps the
stored spec and "" withdraws it. The client record follows the live
spec on every update, and a one-time seeder fills records that still
hold their creation value from the inbound settings, so the editor no
longer posts an empty field that would drop forwards added later.

internal/web/service/client_attach_test.go

@@ -210,8 +210,8 @@ func TestAttachDoesNotCarryForwardedPorts(t *testing.T) {
 				t.Fatalf("GetClients(own): %v", err)
 			}
 			for _, c := range clients {
-				if c.Email == "forwarder" && c.ForwardedPorts != "" {
-					t.Fatalf("attach carried forwardedPorts %q onto the master's inbound", c.ForwardedPorts)
+				if c.Email == "forwarder" && c.ForwardedPortsSpec() != "" {
+					t.Fatalf("attach carried forwardedPorts %q onto the master's inbound", c.ForwardedPortsSpec())
 				}
 			}
 		})

internal/web/service/client_bulk.go

@@ -113,7 +113,7 @@ func (s *ClientService) BulkAttach(inboundSvc *InboundService, emails []string,
 			}
 			client := *rec.ToClient()
 			// Forwards are set per inbound; see Attach.
-			client.ForwardedPorts = ""
+			client.ForwardedPorts = nil
 			if flow, ok := flowsByEmail[rec.Email]; ok && flow != "" {
 				client.Flow = flow
 			}

internal/web/service/client_crud.go

@@ -213,6 +213,10 @@ func (s *ClientService) Create(inboundSvc *InboundService, payload *ClientCreate
 		return false, err
 	}
 	normalizeClientTrafficReset(&client)
+	// Nothing to withdraw on a new client, so a blank spec is no spec.
+	if client.ForwardedPortsSpec() == "" {
+		client.ForwardedPorts = nil
+	}
 	client.ClientHostRuleIds = model.NormalizeClientHostRuleIds(client.ClientHostRuleIds)
 	if len(payload.InboundIds) == 0 {
 		return false, common.NewError("at least one inbound is required")
@@ -938,6 +942,16 @@ func (s *ClientService) Update(inboundSvc *InboundService, id int, updated model
 		return needRestart, err
 	}
 
+	// The AmneziaWG forwards too: the record merge keeps a stored spec when the settings
+	// carry none, so a withdrawal is written here; nil (not sent) leaves it alone.
+	if updated.ForwardedPorts != nil {
+		if err := database.GetDB().Model(&model.ClientRecord{}).
+			Where("id = ?", id).
+			UpdateColumn("wg_forwarded_ports", updated.ForwardedPortsSpec()).Error; err != nil {
+			return needRestart, err
+		}
+	}
+
 	// Same shape as the group write above: SyncInbound keeps a stored ad-tag
 	// when the incoming settings carry none, so clearing the override must be
 	// applied here, where the editor always round-trips the field.
@@ -1186,7 +1200,7 @@ func (s *ClientService) Attach(inboundSvc *InboundService, id int, inboundIds []
 		copyClient := *clientWire
 		// A forward is a listener on the target inbound's host, set per inbound: carried over, it
 		// would let a node-reported client choose ports on this host.
-		copyClient.ForwardedPorts = ""
+		copyClient.ForwardedPorts = nil
 		if !addressesFitAmneziaWGInbound(copyClient.AllowedIPs, inbound) {
 			copyClient.AllowedIPs = nil
 		}

internal/web/service/client_inbound_apply.go

@@ -61,7 +61,7 @@ func validateAmneziaWGClientWrite(inbound *model.Inbound, clients []model.Client
 	inst := amneziawg.Instance{Listen: inbound.Listen, IPv6Enabled: settings.Server.IPv6Enabled}
 	for _, client := range clients {
 		inst.Peers = append(inst.Peers, amneziawg.Peer{
-			AllowedIPs: client.AllowedIPs, ForwardedPorts: client.ForwardedPorts,
+			AllowedIPs: client.AllowedIPs, ForwardedPorts: client.ForwardedPortsSpec(),
 		})
 	}
 	if err := amneziawgkern.ValidateInstanceFeatures(inst, settings.Server.RouteThroughXray); err != nil {
@@ -537,7 +537,7 @@ func (s *ClientService) AddInboundClient(inboundSvc *InboundService, data *model
 			}
 		}
 		if oldInbound.Protocol == model.AmneziaWG {
-			if hit := inboundSvc.checkForwardedPortsConflict(portCtx, client.ForwardedPorts); hit != "" {
+			if hit := inboundSvc.checkForwardedPortsConflict(portCtx, client.ForwardedPortsSpec()); hit != "" {
 				return false, common.NewError("amneziawg: forwardedPorts collides with", hit)
 			}
 		}
@@ -607,7 +607,7 @@ func (s *ClientService) AddInboundClient(inboundSvc *InboundService, data *model
 				return pErr
 			}
 			for i := range clients {
-				if hit := inboundSvc.checkForwardedPortsConflict(txPortCtx, clients[i].ForwardedPorts); hit != "" {
+				if hit := inboundSvc.checkForwardedPortsConflict(txPortCtx, clients[i].ForwardedPortsSpec()); hit != "" {
 					return common.NewError("amneziawg: forwardedPorts collides with", hit)
 				}
 			}
@@ -845,8 +845,8 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo
 		if clients[0].KeepAlive == nil {
 			clients[0].KeepAlive = old.KeepAlive
 		}
-		// AWG partial edits preserve forwarding specs; kernel mode rejects legacy specs.
-		if oldInbound.Protocol == model.AmneziaWG && clients[0].ForwardedPorts == "" {
+		// AWG partial edits (nil: the field was not sent) keep the stored spec; "" withdraws it.
+		if oldInbound.Protocol == model.AmneziaWG && clients[0].ForwardedPorts == nil {
 			clients[0].ForwardedPorts = old.ForwardedPorts
 		}
 	}
@@ -858,7 +858,7 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo
 		if err != nil {
 			return false, err
 		}
-		if hit := inboundSvc.checkForwardedPortsConflict(portCtx, clients[0].ForwardedPorts); hit != "" {
+		if hit := inboundSvc.checkForwardedPortsConflict(portCtx, clients[0].ForwardedPortsSpec()); hit != "" {
 			return false, common.NewError("amneziawg: forwardedPorts collides with", hit)
 		}
 	}
@@ -911,10 +911,14 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo
 				if clients[0].KeepAlive != nil {
 					newMap["keepAlive"] = clients[0].KeepAlive
 				}
-				if oldInbound.Protocol == model.AmneziaWG && clients[0].ForwardedPorts != "" {
-					newMap["forwardedPorts"] = clients[0].ForwardedPorts
+				if oldInbound.Protocol == model.AmneziaWG && clients[0].ForwardedPortsSpec() != "" {
+					newMap["forwardedPorts"] = clients[0].ForwardedPortsSpec()
 				}
 			}
+			// Withdrawn forwards are stored as no spec at all, whatever the protocol.
+			if clients[0].ForwardedPortsSpec() == "" {
+				delete(newMap, "forwardedPorts")
+			}
 			if oldClientMap != nil && sameClientConfigExceptUpdatedAt(oldClientMap, newMap) {
 				if v, ok2 := oldClientMap["updated_at"]; ok2 {
 					newMap["updated_at"] = v
@@ -995,7 +999,7 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo
 			if pErr != nil {
 				return pErr
 			}
-			if hit := inboundSvc.checkForwardedPortsConflict(txPortCtx, clients[0].ForwardedPorts); hit != "" {
+			if hit := inboundSvc.checkForwardedPortsConflict(txPortCtx, clients[0].ForwardedPortsSpec()); hit != "" {
 				return common.NewError("amneziawg: forwardedPorts collides with", hit)
 			}
 		}

internal/web/service/client_link.go

@@ -63,6 +63,11 @@ func applyClientRecordMerge(row *model.ClientRecord, incoming *model.ClientRecor
 	if incoming.KeepAlive != "" {
 		row.KeepAlive = incoming.KeepAlive
 	}
+	// Guarded like Group: only an AmneziaWG entry is sure to carry the spec, and the record
+	// is shared with every other inbound of the client.
+	if incoming.ForwardedPorts != "" {
+		row.ForwardedPorts = incoming.ForwardedPorts
+	}
 	row.SubID = incoming.SubID
 	row.LimitIP = incoming.LimitIP
 	row.TotalGB = incoming.TotalGB

internal/web/service/inbound_amneziawg.go

@@ -324,7 +324,7 @@ func (s *InboundService) normalizeAmneziaWGSettings(inbound *model.Inbound, old
 				if client.Enable {
 					inst.Peers = append(inst.Peers, amneziawg.Peer{
 						AllowedIPs:     client.AllowedIPs,
-						ForwardedPorts: client.ForwardedPorts,
+						ForwardedPorts: client.ForwardedPortsSpec(),
 					})
 				}
 			}
@@ -372,7 +372,7 @@ func (s *InboundService) loadPortConflictContext(db *gorm.DB, nodeID *int) (port
 // 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)
+	hit := s.checkForwardedPortsConflict(ctx, c.ForwardedPortsSpec())
 	if hit == "" {
 		return nil
 	}

internal/web/service/inbound_amneziawg_test.go

@@ -580,3 +580,137 @@ func TestNormalizeAmneziaWGSettingsGatesHostReadinessOnlyWhenEnablingKernelMode(
 		})
 	}
 }
+
+func inboundForwardedPorts(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].ForwardedPortsSpec()
+		}
+	}
+	t.Fatalf("email %q not found on inbound %d", email, ibId)
+	return ""
+}
+
+// seedForwardedPortsClient attaches one AmneziaWG client already holding a
+// port forward, and returns its inbound and client-record id.
+func seedForwardedPortsClient(t *testing.T, email, forwardedPorts string) (*model.Inbound, int) {
+	t.Helper()
+	svc := &ClientService{}
+
+	seeded := model.Client{
+		Email:          email,
+		SubID:          "sub-" + email,
+		Enable:         true,
+		PublicKey:      "QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUE=",
+		AllowedIPs:     []string{"10.8.1.2/32"},
+		ForwardedPorts: model.ForwardedPortsPtr(forwardedPorts),
+	}
+	ib := mkInbound(t, 51830, model.AmneziaWG, clientsSettings(t, []model.Client{seeded}))
+	if err := svc.SyncInbound(nil, ib.Id, []model.Client{seeded}); err != nil {
+		t.Fatalf("seed linkage: %v", err)
+	}
+	return ib, lookupClientRecord(t, email).Id
+}
+
+// The client form sends "" once the field was emptied. Carried forward like an
+// omitted field, the save was reported as done and the port stayed open.
+func TestUpdateCanWithdrawForwardedPorts(t *testing.T) {
+	setupBulkDB(t)
+	inboundSvc := &InboundService{}
+	svc := &ClientService{}
+
+	ib, recId := seedForwardedPortsClient(t, "fp@x", "8022")
+	if got := inboundForwardedPorts(t, inboundSvc, ib.Id, "fp@x"); got != "8022" {
+		t.Fatalf("seeded forwardedPorts = %q, want 8022", got)
+	}
+
+	updated := model.Client{
+		Email:          "fp@x",
+		Enable:         true,
+		AllowedIPs:     []string{"10.8.1.2/32"},
+		ForwardedPorts: model.ForwardedPortsPtr(""),
+	}
+	if _, err := svc.Update(inboundSvc, recId, updated, 0); err != nil {
+		t.Fatalf("Update: %v", err)
+	}
+
+	if got := inboundForwardedPorts(t, inboundSvc, ib.Id, "fp@x"); got != "" {
+		t.Fatalf("inbound forwardedPorts after an explicit \"\" = %q, want none", got)
+	}
+	stored, err := inboundSvc.GetInbound(ib.Id)
+	if err != nil {
+		t.Fatalf("GetInbound: %v", err)
+	}
+	if strings.Contains(stored.Settings, "forwardedPorts") {
+		t.Fatalf("withdrawn forwards must leave no key behind: %s", stored.Settings)
+	}
+	if got := lookupClientRecord(t, "fp@x").ForwardedPorts; got != "" {
+		t.Fatalf("stored wg_forwarded_ports after an explicit \"\" = %q, want none", got)
+	}
+}
+
+// The other half of the contract: a payload that never mentions forwardedPorts
+// (a metadata-only edit from the bot or the API) leaves the stored value alone.
+func TestUpdateWithoutForwardedPortsPreservesTheStoredValue(t *testing.T) {
+	setupBulkDB(t)
+	inboundSvc := &InboundService{}
+	svc := &ClientService{}
+
+	ib, recId := seedForwardedPortsClient(t, "fp@x", "8022")
+
+	updated := model.Client{
+		Email:      "fp@x",
+		Enable:     true,
+		AllowedIPs: []string{"10.8.1.2/32"},
+	}
+	if _, err := svc.Update(inboundSvc, recId, updated, 0); err != nil {
+		t.Fatalf("Update: %v", err)
+	}
+
+	if got := inboundForwardedPorts(t, inboundSvc, ib.Id, "fp@x"); got != "8022" {
+		t.Fatalf("inbound forwardedPorts after an edit that omitted it = %q, want 8022", got)
+	}
+	if got := lookupClientRecord(t, "fp@x").ForwardedPorts; got != "8022" {
+		t.Fatalf("stored wg_forwarded_ports after an edit that omitted it = %q, want 8022", got)
+	}
+}
+
+// The form is filled from the record, and the traffic resets re-save it: a record
+// left at the creation-time spec moves the forward back on the next save.
+func TestUpdateKeepsTheRecordsForwardedPortsCurrent(t *testing.T) {
+	setupBulkDB(t)
+	inboundSvc := &InboundService{}
+	svc := &ClientService{}
+
+	ib, recId := seedForwardedPortsClient(t, "fp@x", "8021")
+
+	updated := model.Client{
+		Email:          "fp@x",
+		Enable:         true,
+		AllowedIPs:     []string{"10.8.1.2/32"},
+		ForwardedPorts: model.ForwardedPortsPtr("8022"),
+	}
+	if _, err := svc.Update(inboundSvc, recId, updated, 0); err != nil {
+		t.Fatalf("Update: %v", err)
+	}
+	rec := lookupClientRecord(t, "fp@x")
+	if rec.ForwardedPorts != "8022" {
+		t.Fatalf("stored wg_forwarded_ports after 8021 -> 8022 = %q, want 8022", rec.ForwardedPorts)
+	}
+
+	if _, err := svc.Update(inboundSvc, recId, *rec.ToClient(), 0); err != nil {
+		t.Fatalf("re-save of the record: %v", err)
+	}
+	if got := inboundForwardedPorts(t, inboundSvc, ib.Id, "fp@x"); got != "8022" {
+		t.Fatalf("inbound forwardedPorts after a re-save of the record = %q, want 8022", got)
+	}
+}

internal/web/service/kernel_client_guard_test.go

@@ -23,7 +23,7 @@ func TestKernelAWGClientWritesRejectUnsafeValuesAndUnsupportedFeatures(t *testin
 		client     model.Client
 	}{
 		{"public key hook injection", "control characters", model.Client{Email: "bad-key@awg", Enable: true, PublicKey: "peer-key\n[Interface]\nPostDown = touch /tmp/pwned", AllowedIPs: []string{"10.8.1.2/32"}}},
-		{"forwarded ports", "port forwarding is not supported", model.Client{Email: "bad-forward@awg", Enable: true, PublicKey: "peer-key", AllowedIPs: []string{"10.8.1.2/32"}, ForwardedPorts: "54321"}},
+		{"forwarded ports", "port forwarding is not supported", model.Client{Email: "bad-forward@awg", Enable: true, PublicKey: "peer-key", AllowedIPs: []string{"10.8.1.2/32"}, ForwardedPorts: model.ForwardedPortsPtr("54321")}},
 		{"IPv6 allowed IP", "per-peer IPv6 egress is not supported", model.Client{Email: "bad-v6@awg", Enable: true, PublicKey: "peer-key", AllowedIPs: []string{"2001:db8::2/128"}}},
 	}
 	for _, tc := range cases {
@@ -70,7 +70,7 @@ func TestKernelAWGClientUpdateRejectsUnsafeValuesAndUnsupportedFeatures(t *testi
 		update     model.Client
 	}{
 		{"preshared key hook injection", "control characters", model.Client{Email: "kept@awg", PreSharedKey: "psk\n[Interface]\nPreDown = touch /tmp/pwned"}},
-		{"forwarded ports", "port forwarding is not supported", model.Client{Email: "kept@awg", ForwardedPorts: "54321"}},
+		{"forwarded ports", "port forwarding is not supported", model.Client{Email: "kept@awg", ForwardedPorts: model.ForwardedPortsPtr("54321")}},
 		{"IPv6 allowed IP", "per-peer IPv6 egress is not supported", model.Client{Email: "kept@awg", AllowedIPs: []string{"2001:db8::2/128"}}},
 	}
 	for _, tc := range cases {
@@ -123,7 +123,7 @@ func TestKernelAWGClientFeatureChecksPreserveSupportedPaths(t *testing.T) {
 		t.Fatalf("valid IPv4 kernel peer was rejected: %v", err)
 	}
 	embedded := mkInbound(t, 52003, model.AmneziaWG, amneziawgClientTestSettings)
-	if _, err := svc.CreateOne(inboundSvc, embedded.Id, model.Client{Email: "forward@embedded", Enable: true, PublicKey: "valid-key", AllowedIPs: []string{"10.8.1.3/32"}, ForwardedPorts: "54321"}); err != nil {
+	if _, err := svc.CreateOne(inboundSvc, embedded.Id, model.Client{Email: "forward@embedded", Enable: true, PublicKey: "valid-key", AllowedIPs: []string{"10.8.1.3/32"}, ForwardedPorts: model.ForwardedPortsPtr("54321")}); err != nil {
 		t.Fatalf("embedded AWG port forwarding was rejected: %v", err)
 	}
 }
@@ -136,7 +136,7 @@ func TestKernelAWGSharedCreateAndUpdateValidateAllTargetsBeforeWrites(t *testing
 		kernel := mkInbound(t, 52005, model.AmneziaWG, kernelAWGSettings(nil))
 		embedded := mkInbound(t, 52006, model.AmneziaWG, amneziawgClientTestSettings)
 		_, err := svc.Create(inboundSvc, &ClientCreatePayload{
-			Client:     model.Client{Email: "shared-create@awg", Enable: true, PublicKey: "valid-key", AllowedIPs: []string{"10.8.1.2/32"}, ForwardedPorts: "54321"},
+			Client:     model.Client{Email: "shared-create@awg", Enable: true, PublicKey: "valid-key", AllowedIPs: []string{"10.8.1.2/32"}, ForwardedPorts: model.ForwardedPortsPtr("54321")},
 			InboundIds: []int{embedded.Id, kernel.Id},
 		})
 		if err == nil || !strings.Contains(err.Error(), "port forwarding is not supported") {
@@ -173,7 +173,7 @@ func TestKernelAWGSharedCreateAndUpdateValidateAllTargetsBeforeWrites(t *testing
 		beforeKernel, _ := inboundSvc.GetInbound(kernel.Id)
 		beforeEmbedded, _ := inboundSvc.GetInbound(embedded.Id)
 		rec := lookupClientRecord(t, seed.Email)
-		_, err := svc.Update(inboundSvc, rec.Id, model.Client{Email: seed.Email, ForwardedPorts: "54321"}, 0)
+		_, err := svc.Update(inboundSvc, rec.Id, model.Client{Email: seed.Email, ForwardedPorts: model.ForwardedPortsPtr("54321")}, 0)
 		if err == nil || !strings.Contains(err.Error(), "port forwarding is not supported") {
 			t.Fatalf("Update error = %v, want kernel feature rejection", err)
 		}