peer: manual port-forward override for UPnP-less networks (#500)

* peer: read PeerManualPortKey setting alongside RADIANCE_PEER_EXTERNAL_PORT

Adds settings.PeerManualPortKey so the user-facing Advanced UI can
persist the manual port forward without an env var. Resolution order
in peer.Client.Start's NewForwarder:

  1. settings.PeerManualPortKey (Advanced UI in lantern Flutter)
  2. RADIANCE_PEER_EXTERNAL_PORT env var (developer / power-user)
  3. UPnP discovery (default)

The setting is wired through lantern-core's
PatchSettings(PeerShareEnabledKey...) path on a separate branch — the
new `setPeerManualPort` FFI export over there calls
PatchSettings({PeerManualPortKey: <int>}) which lands in radiance's
settings store and gets picked up on the next peer.Client.Start.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* portforward: extract manual port forwarder to its own file

The manual port forwarder landed in peer/peer.go via #466 (commit a342889)
to support routers without UPnP. Move it to the portforward package
alongside the UPnP-based Forwarder so every portForwarder implementation
lives in one place.

Net zero functional change, just relocation:

  peer/peer.go
    - manualPortForwarder type + 4 method receivers
    - manualPort() env-parser helper
    - 'strconv' import (no longer needed)
    + NewForwarder closure now calls portforward.NewManualForwarder /
      portforward.ParseManualPort

  peer/peer_test.go
    - TestManualPort + TestManualPortForwarder (moved out of peer pkg)

  portforward/manual.go (new)
    + ManualForwarder + NewManualForwarder
    + ParseManualPort (the env-parser, factored out so callers can decide
      whether to log + fall through or treat as a hard error)
    + MapPort/UnmapPort/StartRenewal/ExternalIP methods
    + 'manual' method tag (was 'manual-env'; dropped the -env suffix
      since this implementation now serves both env and setting paths)

  portforward/manual_test.go (new)
    + TestParseManualPort (9 input cases — boundaries, invalid, empty)
    + TestManualForwarder (full portForwarder contract)

The peer package retains the portForwarder *interface* — that's where
peer expresses what it needs from a forwarder; the concrete implementations
live in portforward.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* peer/portforward/settings: address Copilot review on #500

Four substantive findings; three additional Copilot comments review a
pre-consolidation state of portforward/manual.go that the consolidation
commit (22d1533) replaced wholesale — those are answered with the
relevant context in the thread replies.

1. peer.Client.Start now range-checks the PeerManualPortKey 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 the peer doesn't listen on (or, worse,
   one it does listen on for a different service). Out-of-range values
   are now logged at Warn and fall through to env-var / UPnP as if the
   setting were unset.

2. common/settings PeerManualPortKey doc now documents the 1..65535
   valid range, behavior on out-of-range values, and the 0=unset
   contract. Dropped the peer.Client.Start / portforward.ManualForwarder
   code-location references — describes the contract generically.

3. portforward.NewManualForwarder doc tightened to state the caller-
   side validation contract (port must be 1..65535) without naming
   ParseManualPort or 'env-var path' / 'setting' as callers.

No behavior change in #2 or #3; only #1 changes runtime behavior, and
only for invalid setting values.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* peer/portforward: address Copilot review on #500 (round 2)

Two doc-lint follow-ups per AGENTS.md (no code-location refs in
comments):

1. portforward.ManualForwarder doc dropped the 'satisfies the
   portForwarder contract' phrasing (portForwarder is the peer
   package's private interface; mentioning it crosses a package
   boundary) and the 'peer.Client at it via setting or env var'
   reference. The new wording describes the type in terms of this
   package's own exported API: 'exposes the same Map/Unmap/
   StartRenewal/ExternalIP surface as Forwarder but does no UPnP
   work.'

2. peer.Client.Start's resolution-order comment now spells the
   persisted setting name in quotes ('peer_manual_port') rather than
   the Go identifier (settings.PeerManualPortKey). The persisted
   name is the stable contract — if the Go identifier ever moves or
   renames, the comment stays correct without needing to be updated.
   Same treatment for the env-var line, which already used the
   stable name string.

No behavior change.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Adam Fisk <afisk@mini.local>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: 3 people

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")
+}