peer: manual port-forward override for UPnP-less networks

common/settings/settings.go

@@ -56,6 +56,15 @@ const (
 	AdBlockKey          _key = "ad_block"           // bool
 	AutoConnectKey      _key = "auto_connect"       // bool
 	PeerShareEnabledKey _key = "peer_share_enabled" // bool
+	// PeerManualPortKey is the TCP port number the user has manually
+	// forwarded on their router for the peer-proxy inbound (single-
+	// port 1:1 NAT). Valid range is 1..65535; 0 means unset, in which
+	// case the peer falls back to UPnP discovery. Out-of-range values
+	// (negative, > 65535) are logged on read and treated as unset
+	// rather than silently wrapping to a wrong port. Surfaced as an
+	// Advanced setting in the Share My Connection UI for users on
+	// networks where UPnP is disabled or unavailable.
+	PeerManualPortKey _key = "peer_manual_port" // int (0 = unset; 1..65535 = manual port)
 	SelectedServerKey   _key = "selected_server"    // [servers.Server] Server.Options is not stored
 
 	PreferredLocationKey _key = "preferred_location" // [common.PreferredLocation]

peer/peer.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"log/slog"
 	"math/rand/v2"
-	"strconv"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -17,45 +16,11 @@ import (
 	box "github.com/getlantern/lantern-box"
 	"github.com/getlantern/lantern-box/tracker/peerconn"
 	"github.com/getlantern/radiance/common/env"
+	"github.com/getlantern/radiance/common/settings"
 	"github.com/getlantern/radiance/events"
 	"github.com/getlantern/radiance/portforward"
 )
 
-// manualPortForwarder satisfies the portForwarder interface without doing
-// any UPnP work. Used when env.PeerExternalPort is set.
-type manualPortForwarder struct{ port uint16 }
-
-func (m *manualPortForwarder) MapPort(_ context.Context, _ uint16, _ string) (*portforward.Mapping, error) {
-	return &portforward.Mapping{
-		ExternalPort: m.port,
-		InternalPort: m.port,
-		Method:       "manual-env",
-	}, nil
-}
-func (m *manualPortForwarder) UnmapPort(_ context.Context) error { return nil }
-func (m *manualPortForwarder) StartRenewal(_ context.Context)    {}
-func (m *manualPortForwarder) ExternalIP(_ context.Context) (string, error) {
-	// An empty external IP signals the server to use the address it
-	// observed on the inbound request — when the user has supplied a
-	// manual port but no WAN IP, the server's view is the right answer.
-	return "", nil
-}
-
-// manualPort returns the parsed env.PeerExternalPort value, or 0 if unset
-// or invalid.
-func manualPort() uint16 {
-	raw := env.GetString(env.PeerExternalPort)
-	if raw == "" {
-		return 0
-	}
-	p, err := strconv.Atoi(raw)
-	if err != nil || p < 1 || p > 65535 {
-		slog.Warn("ignoring invalid "+env.PeerExternalPort.String(), "value", raw)
-		return 0
-	}
-	return uint16(p)
-}
-
 // StatusEvent fires whenever the Client's session state changes — successful
 // Start, user Stop, or auto-Stop on a 404 heartbeat.
 type StatusEvent struct {
@@ -209,10 +174,8 @@ func NewClient(cfg Config) (*Client, error) {
 	}
 	if cfg.NewForwarder == nil {
 		cfg.NewForwarder = func(ctx context.Context) (portForwarder, error) {
-			if p := manualPort(); p != 0 {
-				slog.Info("peer client using manual port forward",
-					"port", p, "env", env.PeerExternalPort.String())
-				return &manualPortForwarder{port: p}, nil
+			if fwd := pickManualForwarder(); fwd != nil {
+				return fwd, nil
 			}
 			// Explicitly return a nil interface on error — `return
 			// portforward.NewForwarder(ctx)` collapses the (*Forwarder, error)
@@ -647,6 +610,53 @@ func ensurePeerOutboundsBypassVPN(options string) (string, error) {
 	return string(out), nil
 }
 
+// pickManualForwarder resolves the manual port override against the
+// two configured sources and returns a ManualForwarder, or nil if
+// neither source supplies a valid port. The default NewForwarder
+// factory in NewClient calls this first; nil means "fall through to
+// UPnP discovery."
+//
+// Resolution order:
+//
+//  1. "peer_manual_port" setting (Advanced UI)
+//  2. RADIANCE_PEER_EXTERNAL_PORT env var (developer / power-user)
+//  3. nil — caller falls through to UPnP
+//
+// Persisted names are quoted so the comment stays accurate if Go
+// identifiers move or rename.
+//
+// The setting is range-checked before casting to uint16 — a raw cast
+// silently wraps negative values (-5 → 65531) and values above the
+// port space (70000 → 4464), which would register a port the peer
+// doesn't listen on (or, worse, one it does listen on for another
+// service). Out-of-range / unparseable values are logged at Warn and
+// the resolution falls through to the next source as if unset.
+func pickManualForwarder() portForwarder {
+	if raw := settings.GetInt(settings.PeerManualPortKey); raw != 0 {
+		if raw < 1 || raw > 65535 {
+			slog.Warn("ignoring out-of-range peer_manual_port setting; falling through to env / UPnP",
+				"value", raw)
+		} else {
+			port := uint16(raw)
+			slog.Info("peer client using manual port forward",
+				"port", port, "source", "setting")
+			return portforward.NewManualForwarder(port)
+		}
+	}
+	if raw := env.GetString(env.PeerExternalPort); raw != "" {
+		port, err := portforward.ParseManualPort(raw)
+		if err != nil {
+			slog.Warn("ignoring invalid "+env.PeerExternalPort.String(),
+				"value", raw, "err", err)
+		} else {
+			slog.Info("peer client using manual port forward",
+				"port", port, "source", env.PeerExternalPort.String())
+			return portforward.NewManualForwarder(port)
+		}
+	}
+	return nil
+}
+
 func pickInternalPort() uint16 {
 	return uint16(internalPortMin + rand.IntN(internalPortMax-internalPortMin))
 }

peer/peer_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/getlantern/radiance/common"
+	"github.com/getlantern/radiance/common/settings"
 	"github.com/getlantern/radiance/events"
 	"github.com/getlantern/radiance/portforward"
 )
@@ -627,58 +628,55 @@ func TestPickInternalPort_InRange(t *testing.T) {
 	}
 }
 
-// manualPort parses the RADIANCE_PEER_EXTERNAL_PORT env var. Unset, empty,
-// non-numeric, and out-of-range values all collapse to 0, which the
-// NewClient default factory treats as "no override → use UPnP discovery".
-// Only a 1..65535 value selects the manual path.
-func TestManualPort(t *testing.T) {
+// pickManualForwarder is the default-NewForwarder factory's first
+// branch: setting → env-var → nil (= caller falls through to UPnP).
+// Tests each resolution path and the out-of-range / unparseable
+// fallthrough behavior. Out-of-range setting + unset env returns nil
+// — peer.NewClient's caller treats that as "use UPnP discovery."
+func TestPickManualForwarder(t *testing.T) {
 	tests := []struct {
-		name string
-		env  string
-		want uint16
+		name       string
+		setting    int    // 0 means unset
+		envVar     string // "" means unset
+		wantManual bool
+		wantPort   uint16
 	}{
-		{"unset", "", 0},
-		{"valid mid-range", "5698", 5698},
-		{"valid low boundary", "1", 1},
-		{"valid high boundary", "65535", 65535},
-		{"non-numeric", "abc", 0},
-		{"zero", "0", 0},
-		{"negative", "-5", 0},
-		{"above uint16", "65536", 0},
-		{"way above uint16", "99999", 0},
+		{"setting takes precedence over env", 5698, "1234", true, 5698},
+		{"setting only", 5698, "", true, 5698},
+		{"env only", 0, "5698", true, 5698},
+		{"both unset → fall through", 0, "", false, 0},
+		{"setting out of range, env unset → fall through", 70000, "", false, 0},
+		{"setting negative, env unset → fall through", -5, "", false, 0},
+		{"setting out of range, env valid → env wins", 70000, "5698", true, 5698},
+		{"setting unset, env unparseable → fall through", 0, "abc", false, 0},
+		{"setting valid low boundary", 1, "", true, 1},
+		{"setting valid high boundary", 65535, "", true, 65535},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			t.Setenv("RADIANCE_PEER_EXTERNAL_PORT", tc.env)
-			assert.Equal(t, tc.want, manualPort())
+			require.NoError(t, settings.InitSettings(t.TempDir()))
+			t.Cleanup(settings.Reset)
+			if tc.setting != 0 {
+				require.NoError(t, settings.Set(settings.PeerManualPortKey, tc.setting))
+			}
+			t.Setenv("RADIANCE_PEER_EXTERNAL_PORT", tc.envVar)
+
+			fwd := pickManualForwarder()
+			if !tc.wantManual {
+				assert.Nil(t, fwd, "expected fall-through (nil) but got a forwarder")
+				return
+			}
+			require.NotNil(t, fwd, "expected manual forwarder, got nil")
+			// Verify the chosen port via the public MapPort surface —
+			// ManualForwarder.port is unexported, MapPort echoes it.
+			mapping, err := fwd.MapPort(context.Background(), 0, "")
+			require.NoError(t, err)
+			assert.Equal(t, tc.wantPort, mapping.ExternalPort)
+			assert.Equal(t, tc.wantPort, mapping.InternalPort)
 		})
 	}
 }
 
-// manualPortForwarder must satisfy the portForwarder contract: MapPort
-// returns a Mapping using the configured port for both internal and
-// external (no rewrite — that's the user's responsibility), UnmapPort
-// and StartRenewal are no-ops, and ExternalIP returns "" so the server
-// substitutes the IP it observed on the request.
-func TestManualPortForwarder(t *testing.T) {
-	f := &manualPortForwarder{port: 5698}
-
-	m, err := f.MapPort(context.Background(), 30001, "ignored")
-	require.NoError(t, err)
-	assert.Equal(t, uint16(5698), m.ExternalPort)
-	assert.Equal(t, uint16(5698), m.InternalPort, "external==internal — user mapped them themselves")
-	assert.Equal(t, "manual-env", m.Method)
-
-	require.NoError(t, f.UnmapPort(context.Background()), "UnmapPort is a no-op for manual forwarders")
-
-	// StartRenewal must not panic or block.
-	f.StartRenewal(context.Background())
-
-	ip, err := f.ExternalIP(context.Background())
-	require.NoError(t, err)
-	assert.Empty(t, ip, "empty ip signals server to use observed source address")
-}
-
 func TestAPIError_StringFormat(t *testing.T) {
 	e := &APIError{Status: 422, Body: "could not connect to peer port"}
 	assert.Contains(t, e.Error(), "422")

portforward/manual.go

@@ -0,0 +1,84 @@
+package portforward
+
+import (
+	"context"
+	"fmt"
+	"strconv"
+)
+
+// ManualForwarder exposes the same Map/Unmap/StartRenewal/ExternalIP
+// surface as Forwarder but does no UPnP work. The user is expected to
+// have configured a port forward on their router by hand and supplied
+// the port number out-of-band. This implementation reports the same
+// value for both the external and internal port — callers needing
+// distinct external/internal ports should use the UPnP-based Forwarder
+// (which can negotiate them) or build their own portForwarder.
+//
+// Use case: networks where UPnP is disabled or unavailable (router has
+// UPnP off for security, ISP-provided gateways without IGD, networks
+// behind double-NAT). The UPnP-based Forwarder fails in those
+// environments; this type lets callers bypass discovery entirely.
+type ManualForwarder struct {
+	port uint16
+}
+
+// NewManualForwarder builds a ManualForwarder for a pre-configured
+// router port forward. port must be in 1..65535; the caller is
+// responsible for validating its input before calling.
+func NewManualForwarder(port uint16) *ManualForwarder {
+	return &ManualForwarder{port: port}
+}
+
+// ParseManualPort parses a string into a TCP port number. Values outside
+// 1..65535 return an error so callers can log and fall through to UPnP
+// discovery rather than register a non-listening port with the server.
+func ParseManualPort(s string) (uint16, error) {
+	p, err := strconv.Atoi(s)
+	if err != nil {
+		return 0, fmt.Errorf("parse %q: %w", s, err)
+	}
+	if p < 1 || p > 65535 {
+		return 0, fmt.Errorf("port %d out of range (1..65535)", p)
+	}
+	return uint16(p), nil
+}
+
+// MapPort reports the configured port as both external and internal. The
+// router-side rule is already in place; nothing to do at the protocol
+// layer. Returns an error if the forwarder was constructed with port==0
+// (not a valid listen/registration port) so callers can fall through to
+// UPnP rather than register a port no peer will listen on.
+//
+// Protocol is set to "TCP" to match the UPnP-based Forwarder, which
+// hard-codes the same value. Samizdat-in inbound traffic is TCP-only;
+// when UDP support is added the two forwarders should be widened
+// together.
+func (m *ManualForwarder) MapPort(_ context.Context, _ uint16, _ string) (*Mapping, error) {
+	if m.port == 0 {
+		return nil, fmt.Errorf("manual forwarder constructed with port=0")
+	}
+	return &Mapping{
+		ExternalPort: m.port,
+		InternalPort: m.port,
+		Protocol:     "TCP",
+		Method:       "manual",
+	}, nil
+}
+
+// UnmapPort is a no-op: the user owns the router rule and is responsible
+// for removing it.
+func (m *ManualForwarder) UnmapPort(_ context.Context) error { return nil }
+
+// StartRenewal is a no-op: manually-configured rules don't carry a UPnP
+// lease and don't need refreshing.
+func (m *ManualForwarder) StartRenewal(_ context.Context) {}
+
+// ExternalIP returns the empty string deliberately. With a manual port
+// forward we have no UPnP gateway to ask for the WAN address, and
+// probing a public IP service from the client adds a network roundtrip
+// for information lantern-cloud already has — the server observes the
+// peer's source address on the register call and uses that as the
+// canonical external IP when this field is empty.
+func (m *ManualForwarder) ExternalIP(_ context.Context) (string, error) {
+	return "", nil
+}