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,52 @@ 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
+			// Manual port-forward override. Use case: networks where
+			// UPnP is disabled or unavailable (router has UPnP off for
+			// security, ISP-provided gateways without IGD, networks
+			// behind double-NAT) but the user has manually configured
+			// a port forward on their router. We trust the user's
+			// config — no UPnP roundtrip — and report the configured
+			// port as both the external and internal port (the 1:1
+			// case every consumer router exposes).
+			//
+			// Resolution order:
+			//   1. "peer_manual_port" setting (Advanced UI)
+			//   2. RADIANCE_PEER_EXTERNAL_PORT env var (developer /
+			//      power-user override)
+			//   3. fall through to UPnP discovery
+			//
+			// Persisted names are quoted so the comment stays accurate
+			// if Go identifiers move or rename.
+			//
+			// Range-check the setting before casting to uint16 — a raw
+			// uint16 cast silently wraps negative values (-5 → 65531)
+			// and values above the port space (70000 → 4464), which
+			// would register a port we don't listen on (or, worse, one
+			// we do listen on for a different service). Out-of-range
+			// values fall through to env-var and then UPnP as if the
+			// setting were unset.
+			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), nil
+				}
+			}
+			if raw := env.GetString(env.PeerExternalPort); raw != "" {
+				port, err := portforward.ParseManualPort(raw)
+				if err != nil {
+					slog.Warn("ignoring invalid "+env.PeerExternalPort.String(),
+						"value", raw, "error", err)
+				} else {
+					slog.Info("peer client using manual port forward",
+						"port", port, "source", env.PeerExternalPort.String())
+					return portforward.NewManualForwarder(port), nil
+				}
 			}
 			// Explicitly return a nil interface on error — `return
 			// portforward.NewForwarder(ctx)` collapses the (*Forwarder, error)

peer/peer_test.go

@@ -627,58 +627,6 @@ 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) {
-	tests := []struct {
-		name string
-		env  string
-		want 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},
-	}
-	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())
-		})
-	}
-}
-
-// 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,71 @@
+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 (single-port
+// 1:1 NAT — every consumer router exposes port forwarding as a single
+// port number) and supplied the port number out-of-band.
+//
+// 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.
+func (m *ManualForwarder) MapPort(_ context.Context, _ uint16, _ string) (*Mapping, error) {
+	return &Mapping{
+		ExternalPort: m.port,
+		InternalPort: m.port,
+		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
+}

portforward/manual_test.go

@@ -0,0 +1,66 @@
+package portforward
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// ParseManualPort accepts 1..65535 verbatim and rejects everything else
+// with an error so callers can log + fall through to UPnP discovery
+// rather than register a non-listening port with lantern-cloud.
+func TestParseManualPort(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   string
+		want    uint16
+		wantErr bool
+	}{
+		{"valid mid-range", "5698", 5698, false},
+		{"valid low boundary", "1", 1, false},
+		{"valid high boundary", "65535", 65535, false},
+		{"empty", "", 0, true},
+		{"non-numeric", "abc", 0, true},
+		{"zero", "0", 0, true},
+		{"negative", "-5", 0, true},
+		{"above uint16", "65536", 0, true},
+		{"way above uint16", "99999", 0, true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := ParseManualPort(tc.input)
+			if tc.wantErr {
+				assert.Error(t, err)
+				assert.Equal(t, uint16(0), got)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+// ManualForwarder satisfies the portForwarder contract: MapPort returns
+// a Mapping with external==internal port and the "manual" method tag,
+// UnmapPort and StartRenewal are no-ops, ExternalIP returns "" so the
+// server substitutes the IP it observed on the register call.
+func TestManualForwarder(t *testing.T) {
+	f := NewManualForwarder(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", 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")
+}