From a9191cb468f3248ea9b1d77521f6d8216a204319 Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sun, 1 Nov 2026 08:55:38 +0100 Subject: [PATCH] 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. --- docs/public/openapi.json | 3 +- frontend/public/openapi.json | 3 +- frontend/src/generated/examples.ts | 2 +- frontend/src/generated/schemas.ts | 3 +- frontend/src/generated/types.ts | 2 +- frontend/src/generated/zod.ts | 2 +- internal/amneziawg/instance.go | 2 +- internal/database/db.go | 42 +++++- internal/database/db_seed_test.go | 40 ++++++ internal/database/model/model.go | 27 +++- .../database/model/model_wireguard_test.go | 12 ++ internal/web/service/client_attach_test.go | 4 +- internal/web/service/client_bulk.go | 2 +- internal/web/service/client_crud.go | 16 ++- internal/web/service/client_inbound_apply.go | 22 +-- internal/web/service/client_link.go | 5 + internal/web/service/inbound_amneziawg.go | 4 +- .../web/service/inbound_amneziawg_test.go | 134 ++++++++++++++++++ .../web/service/kernel_client_guard_test.go | 10 +- 19 files changed, 304 insertions(+), 31 deletions(-) diff --git a/docs/public/openapi.json b/docs/public/openapi.json index 9b90660cc..5b2aa2488 100644 --- a/docs/public/openapi.json +++ b/docs/public/openapi.json @@ -1785,7 +1785,8 @@ "type": "string" }, "forwardedPorts": { - "description": "AmneziaWG per-client port-forwarding spec, e.g. \"80,443,8000-8100\"", + "description": "AmneziaWG port-forwarding spec, e.g. \"80,443,8000-8100\"; \"\" withdraws the forwards, omit to keep them", + "nullable": true, "type": "string" }, "group": { diff --git a/frontend/public/openapi.json b/frontend/public/openapi.json index 9b90660cc..5b2aa2488 100644 --- a/frontend/public/openapi.json +++ b/frontend/public/openapi.json @@ -1785,7 +1785,8 @@ "type": "string" }, "forwardedPorts": { - "description": "AmneziaWG per-client port-forwarding spec, e.g. \"80,443,8000-8100\"", + "description": "AmneziaWG port-forwarding spec, e.g. \"80,443,8000-8100\"; \"\" withdraws the forwards, omit to keep them", + "nullable": true, "type": "string" }, "group": { diff --git a/frontend/src/generated/examples.ts b/frontend/src/generated/examples.ts index 6c8c5aca5..384de907c 100644 --- a/frontend/src/generated/examples.ts +++ b/frontend/src/generated/examples.ts @@ -431,7 +431,7 @@ export const EXAMPLES: Record = { "enable": false, "expiryTime": 0, "flow": "", - "forwardedPorts": "", + "forwardedPorts": null, "group": "", "id": "", "keepAlive": 0, diff --git a/frontend/src/generated/schemas.ts b/frontend/src/generated/schemas.ts index 8165b101b..479ec9fdb 100644 --- a/frontend/src/generated/schemas.ts +++ b/frontend/src/generated/schemas.ts @@ -1759,7 +1759,8 @@ export const SCHEMAS: Record = { "type": "string" }, "forwardedPorts": { - "description": "AmneziaWG per-client port-forwarding spec, e.g. \"80,443,8000-8100\"", + "description": "AmneziaWG port-forwarding spec, e.g. \"80,443,8000-8100\"; \"\" withdraws the forwards, omit to keep them", + "nullable": true, "type": "string" }, "group": { diff --git a/frontend/src/generated/types.ts b/frontend/src/generated/types.ts index a7e158e36..c9d50cfc6 100644 --- a/frontend/src/generated/types.ts +++ b/frontend/src/generated/types.ts @@ -426,7 +426,7 @@ export interface Client { enable: boolean; expiryTime: number; flow?: string; - forwardedPorts?: string; + forwardedPorts?: string | null; group?: string; id?: string; keepAlive?: number | string; diff --git a/frontend/src/generated/zod.ts b/frontend/src/generated/zod.ts index 692a2f677..d86fa6391 100644 --- a/frontend/src/generated/zod.ts +++ b/frontend/src/generated/zod.ts @@ -450,7 +450,7 @@ export const ClientSchema = z.object({ enable: z.boolean(), expiryTime: z.number().int(), flow: z.string().optional(), - forwardedPorts: z.string().optional(), + forwardedPorts: z.string().nullable().optional(), group: z.string().optional(), id: z.string().optional(), keepAlive: z.union([z.number().int(), z.string()]).optional(), diff --git a/internal/amneziawg/instance.go b/internal/amneziawg/instance.go index 8c75af6ed..81a58823d 100644 --- a/internal/amneziawg/instance.go +++ b/internal/amneziawg/instance.go @@ -34,7 +34,7 @@ func InstanceFromInbound(ib *model.Inbound) (Instance, bool) { PublicKey: c.PublicKey, PresharedKey: c.PreSharedKey, AllowedIPs: c.AllowedIPs, - ForwardedPorts: c.ForwardedPorts, + ForwardedPorts: c.ForwardedPortsSpec(), }) } if len(peers) == 0 { diff --git a/internal/database/db.go b/internal/database/db.go index 80b680365..f3cbb7c47 100644 --- a/internal/database/db.go +++ b/internal/database/db.go @@ -1530,7 +1530,7 @@ func runSeeders(isUsersEmpty bool) error { } if empty && isUsersEmpty { - seeders := []string{"UserPasswordHash", "ClientsTable", "InboundClientsArrayFix", "InboundClientTgIdFix2", "InboundClientSubIdFix", "FreedomFinalRulesReverseFix", "FreedomFinalRulesPrivateEgressBlock", "UppercaseFreedomFinalRulesFix", "InboundRealityFinalmaskTcpStrip", "ApiTokensHash", "LegacyProxySettingsCleanup", "SessionSecretRotation", "OutboundRemovedKeysFix", "FreedomDomainStrategyFix", "DNSOutboundLegacyKeysFix", "DNSOutboundQTypeZeroFix", "WireguardDomainStrategyFix", "XdnsFinalmaskNamesAddrsFix", "UDPHopClientMaskFix", "HostVerifyPeerCertByNameBoolFix", "HostMihomoX25519Reset", "WireguardPeersToClients", "MtprotoSecretsToClients", "NodeInboundsAdopted", "NodeDownNotified", "ResetIpLimitNoFail2ban", "ClientGroupNameTrim", "OwnerlessClientHwidPurge"} + seeders := []string{"UserPasswordHash", "ClientsTable", "InboundClientsArrayFix", "InboundClientTgIdFix2", "InboundClientSubIdFix", "FreedomFinalRulesReverseFix", "FreedomFinalRulesPrivateEgressBlock", "UppercaseFreedomFinalRulesFix", "InboundRealityFinalmaskTcpStrip", "ApiTokensHash", "LegacyProxySettingsCleanup", "SessionSecretRotation", "OutboundRemovedKeysFix", "FreedomDomainStrategyFix", "DNSOutboundLegacyKeysFix", "DNSOutboundQTypeZeroFix", "WireguardDomainStrategyFix", "XdnsFinalmaskNamesAddrsFix", "UDPHopClientMaskFix", "HostVerifyPeerCertByNameBoolFix", "HostMihomoX25519Reset", "WireguardPeersToClients", "MtprotoSecretsToClients", "NodeInboundsAdopted", "NodeDownNotified", "ResetIpLimitNoFail2ban", "ClientGroupNameTrim", "OwnerlessClientHwidPurge", "ClientForwardedPortsBackfill"} for _, name := range seeders { if err := db.Create(&model.HistoryOfSeeders{SeederName: name}).Error; err != nil { return err @@ -1725,6 +1725,12 @@ func runSeeders(isUsersEmpty bool) error { } } + if !slices.Contains(seedersHistory, "ClientForwardedPortsBackfill") { + if err := backfillClientForwardedPorts(); err != nil { + return err + } + } + if err := seedHostsFromExternalProxy(); err != nil { return err } @@ -1920,6 +1926,40 @@ func clearLegacyProxySettings() error { }) } +// The client editor fills forwardedPorts from the record, which kept the value it was created +// with; an emptied field now withdraws the forwards, so the record must hold the live spec. +func backfillClientForwardedPorts() error { + return db.Transaction(func(tx *gorm.DB) error { + var inbounds []model.Inbound + if err := tx.Select("id", "settings").Where("protocol = ?", model.AmneziaWG).Find(&inbounds).Error; err != nil { + return err + } + for _, ib := range inbounds { + var parsed struct { + Clients []struct { + Email string `json:"email"` + ForwardedPorts string `json:"forwardedPorts"` + } `json:"clients"` + } + if json.Unmarshal([]byte(ib.Settings), &parsed) != nil { + continue + } + for _, c := range parsed.Clients { + spec := strings.TrimSpace(c.ForwardedPorts) + if c.Email == "" || spec == "" { + continue + } + if err := tx.Model(&model.ClientRecord{}). + Where("email = ? AND (wg_forwarded_ports IS NULL OR wg_forwarded_ports = '')", c.Email). + UpdateColumn("wg_forwarded_ports", spec).Error; err != nil { + return err + } + } + } + return tx.Create(&model.HistoryOfSeeders{SeederName: "ClientForwardedPortsBackfill"}).Error + }) +} + // The HWID gate stored a row for any subId a request named, owned or not; those rows // belong to no subscription and only grow the database. func purgeOwnerlessClientHwids() error { diff --git a/internal/database/db_seed_test.go b/internal/database/db_seed_test.go index 8bf177254..e42be24be 100644 --- a/internal/database/db_seed_test.go +++ b/internal/database/db_seed_test.go @@ -452,3 +452,43 @@ func TestOwnerlessClientHwidsPurgedOnUpgrade(t *testing.T) { t.Fatalf("hwid rows after upgrade = %q, want [owned]", subs) } } + +// The editor fills forwardedPorts from the record, and an emptied field now withdraws the +// forwards: a record left at its creation value would drop live forwards on the next save. +func TestClientForwardedPortsBackfilledOnUpgrade(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "x-ui.db") + if err := InitDB(dbPath); err != nil { + t.Fatalf("InitDB: %v", err) + } + t.Cleanup(func() { _ = CloseDB() }) + if err := db.Create(&model.ClientRecord{Email: "fwd@x", Enable: true}).Error; err != nil { + t.Fatalf("seed client: %v", err) + } + if err := db.Create(&model.ClientRecord{Email: "kept@x", Enable: true, ForwardedPorts: "9000"}).Error; err != nil { + t.Fatalf("seed client: %v", err) + } + if err := db.Create(&model.Inbound{ + Tag: "awg", Port: 51820, Protocol: model.AmneziaWG, Enable: true, + Settings: `{"clients":[{"email":"fwd@x","forwardedPorts":"8022"},{"email":"kept@x","forwardedPorts":"9001"}]}`, + }).Error; err != nil { + t.Fatalf("seed inbound: %v", err) + } + if err := db.Where("seeder_name = ?", "ClientForwardedPortsBackfill").Delete(&model.HistoryOfSeeders{}).Error; err != nil { + t.Fatalf("clear seeder history: %v", err) + } + if err := CloseDB(); err != nil { + t.Fatalf("CloseDB: %v", err) + } + if err := InitDB(dbPath); err != nil { + t.Fatalf("reopen InitDB: %v", err) + } + for email, want := range map[string]string{"fwd@x": "8022", "kept@x": "9000"} { + var rec model.ClientRecord + if err := db.Where("email = ?", email).First(&rec).Error; err != nil { + t.Fatalf("read %s: %v", email, err) + } + if rec.ForwardedPorts != want { + t.Errorf("%s forwardedPorts = %q after upgrade, want %q", email, rec.ForwardedPorts, want) + } + } +} diff --git a/internal/database/model/model.go b/internal/database/model/model.go index cf93ba7fe..06120d9f9 100644 --- a/internal/database/model/model.go +++ b/internal/database/model/model.go @@ -955,7 +955,7 @@ type Client struct { AllowedIPsByInbound map[int][]string `json:"allowedIPsByInbound,omitempty"` PreSharedKey string `json:"preSharedKey,omitempty"` KeepAlive *KeepAliveValue `json:"keepAlive,omitempty"` // Numeric values or AmneziaWG ranges; 0 disables, omission preserves. - ForwardedPorts string `json:"forwardedPorts,omitempty"` // AmneziaWG per-client port-forwarding spec, e.g. "80,443,8000-8100" + ForwardedPorts *string `json:"forwardedPorts,omitempty"` // AmneziaWG port-forwarding spec, e.g. "80,443,8000-8100"; "" withdraws the forwards, omit to keep them Secret string `json:"secret,omitempty" example:"ee1234567890abcdef1234567890abcd7777772e636c6f7564666c6172652e636f6d"` AdTag string `json:"adTag,omitempty" example:"0123456789abcdef0123456789abcdef"` Email string `json:"email"` // Client email identifier @@ -1273,6 +1273,27 @@ func keepAliveValue(value *KeepAliveValue) KeepAliveValue { return *value } +// ForwardedPortsSpec is the client's port-forwarding spec, "" when unset. +func (c Client) ForwardedPortsSpec() string { + if c.ForwardedPorts == nil { + return "" + } + return strings.TrimSpace(*c.ForwardedPorts) +} + +// ForwardedPortsPtr wraps an explicit spec, "" included: distinct from a nil +// ForwardedPorts, which means the field was never sent. +func ForwardedPortsPtr(spec string) *string { return &spec } + +// nonEmptyForwardedPorts keeps a stored "" nil: a record re-saved as a client must not +// read as a request to withdraw the forwards. +func nonEmptyForwardedPorts(spec string) *string { + if spec == "" { + return nil + } + return ForwardedPortsPtr(spec) +} + func (c *Client) ToRecord() *ClientRecord { rec := &ClientRecord{ Email: c.Email, @@ -1304,7 +1325,7 @@ func (c *Client) ToRecord() *ClientRecord { AllowedIPs: strings.Join(c.AllowedIPs, ","), PreSharedKey: c.PreSharedKey, KeepAlive: keepAliveValue(c.KeepAlive), - ForwardedPorts: c.ForwardedPorts, + ForwardedPorts: c.ForwardedPortsSpec(), Secret: c.Secret, AdTag: c.AdTag, } @@ -1364,7 +1385,7 @@ func (r *ClientRecord) ToClient() *Client { AllowedIPs: splitWireguardAllowedIPs(r.AllowedIPs), PreSharedKey: r.PreSharedKey, KeepAlive: keepAlivePtr(r.KeepAlive), - ForwardedPorts: r.ForwardedPorts, + ForwardedPorts: nonEmptyForwardedPorts(r.ForwardedPorts), Secret: r.Secret, AdTag: r.AdTag, } diff --git a/internal/database/model/model_wireguard_test.go b/internal/database/model/model_wireguard_test.go index f2d7b8898..10c0c7ee9 100644 --- a/internal/database/model/model_wireguard_test.go +++ b/internal/database/model/model_wireguard_test.go @@ -16,6 +16,8 @@ func TestClientToRecordRoundTripWireGuard(t *testing.T) { AllowedIPs: []string{"10.0.0.2/32", "fd00::2/128"}, PreSharedKey: "cHNrLWJhc2U2NC0zMmJ5dGVzLXBsYWNlaG9sZGVyISE=", KeepAlive: KeepAlivePtr(25), + + ForwardedPorts: ForwardedPortsPtr("80,8000-8100"), } rec := c.ToRecord() @@ -32,6 +34,7 @@ func TestClientToRecordRoundTripWireGuard(t *testing.T) { {"PublicKey", c.PublicKey, got.PublicKey}, {"PreSharedKey", c.PreSharedKey, got.PreSharedKey}, {"KeepAlive", c.KeepAliveSeconds(), got.KeepAliveSeconds()}, + {"ForwardedPorts", c.ForwardedPortsSpec(), got.ForwardedPortsSpec()}, } { if f.a != f.b { t.Errorf("%s round-trip = %v, want %v", f.name, f.b, f.a) @@ -59,6 +62,15 @@ func TestClientToClientOmitsUnsetKeepAlive(t *testing.T) { } } +// An explicit "" withdraws the forwards, which a re-saved record must never ask for. +func TestClientToClientOmitsUnsetForwardedPorts(t *testing.T) { + rec := &ClientRecord{Email: "awg@example.test", Enable: true} + + if got := rec.ToClient().ForwardedPorts; got != nil { + t.Fatalf("ForwardedPorts for a record that never set any = %q, want nil", *got) + } +} + func TestClientRecordEmptyAllowedIPs(t *testing.T) { rec := &ClientRecord{Email: "bob@example.test", AllowedIPs: ""} if got := rec.ToClient().AllowedIPs; got != nil { diff --git a/internal/web/service/client_attach_test.go b/internal/web/service/client_attach_test.go index 65000a403..45c123c2b 100644 --- a/internal/web/service/client_attach_test.go +++ b/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()) } } }) diff --git a/internal/web/service/client_bulk.go b/internal/web/service/client_bulk.go index f6fe94586..80dba5f38 100644 --- a/internal/web/service/client_bulk.go +++ b/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 } diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index 904f93037..e2878159e 100644 --- a/internal/web/service/client_crud.go +++ b/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 } diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index cc563cdf2..f1142d87f 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/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) } } diff --git a/internal/web/service/client_link.go b/internal/web/service/client_link.go index 912408bb2..fe69c523b 100644 --- a/internal/web/service/client_link.go +++ b/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 diff --git a/internal/web/service/inbound_amneziawg.go b/internal/web/service/inbound_amneziawg.go index e4e099ea2..c32064198 100644 --- a/internal/web/service/inbound_amneziawg.go +++ b/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 } diff --git a/internal/web/service/inbound_amneziawg_test.go b/internal/web/service/inbound_amneziawg_test.go index 94c057d52..2a2d48df3 100644 --- a/internal/web/service/inbound_amneziawg_test.go +++ b/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) + } +} diff --git a/internal/web/service/kernel_client_guard_test.go b/internal/web/service/kernel_client_guard_test.go index 7e6d15547..f6c2a5e2b 100644 --- a/internal/web/service/kernel_client_guard_test.go +++ b/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) }