diff --git a/internal/web/service/inbound.go b/internal/web/service/inbound.go index 31fc22e7c..fb8450d46 100644 --- a/internal/web/service/inbound.go +++ b/internal/web/service/inbound.go @@ -1257,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 diff --git a/internal/web/service/inbound_amneziawg.go b/internal/web/service/inbound_amneziawg.go index 5e1a9ce3d..0e961403f 100644 --- a/internal/web/service/inbound_amneziawg.go +++ b/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) @@ -333,6 +333,35 @@ func (s *InboundService) loadPortConflictContext(db *gorm.DB) (portConflictConte return ctx, err } +// 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 { diff --git a/internal/web/service/inbound_amneziawg_relay_window_test.go b/internal/web/service/inbound_amneziawg_relay_window_test.go index 41ec5a85d..d8b66ebe2 100644 --- a/internal/web/service/inbound_amneziawg_relay_window_test.go +++ b/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,31 @@ 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) {