From ab2976e2a19b2786f394e22265126c777af4280b Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Thu, 7 May 2026 14:28:47 -0600 Subject: [PATCH 1/4] peer: read PeerManualPortKey setting alongside RADIANCE_PEER_EXTERNAL_PORT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: }) 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) --- common/settings/settings.go | 7 +++++++ peer/peer.go | 22 +++++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/common/settings/settings.go b/common/settings/settings.go index 3a3afd90..1c5e5cb1 100644 --- a/common/settings/settings.go +++ b/common/settings/settings.go @@ -56,6 +56,13 @@ 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 (single-port 1:1 NAT). When non-zero, + // peer.Client.Start uses portforward.ManualForwarder with this port + // instead of probing UPnP. 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 = use UPnP) SelectedServerKey _key = "selected_server" // [servers.Server] Server.Options is not stored PreferredLocationKey _key = "preferred_location" // [common.PreferredLocation] diff --git a/peer/peer.go b/peer/peer.go index c9b87281..0aec7b9e 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -17,6 +17,7 @@ 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" ) @@ -209,9 +210,28 @@ func NewClient(cfg Config) (*Client, error) { } if cfg.NewForwarder == nil { cfg.NewForwarder = func(ctx context.Context) (portForwarder, error) { + // 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. settings.PeerManualPortKey (Advanced UI) + // 2. RADIANCE_PEER_EXTERNAL_PORT env var (developer / + // power-user override) + // 3. fall through to UPnP discovery + if port := uint16(settings.GetInt(settings.PeerManualPortKey)); port != 0 { + slog.Info("peer client using manual port forward", + "port", port, "source", "setting") + return &manualPortForwarder{port: port}, nil + } if p := manualPort(); p != 0 { slog.Info("peer client using manual port forward", - "port", p, "env", env.PeerExternalPort.String()) + "port", p, "source", env.PeerExternalPort.String()) return &manualPortForwarder{port: p}, nil } // Explicitly return a nil interface on error — `return From 8667d26857c4e885f208e65c654c48846f0f8fe4 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Fri, 29 May 2026 15:14:28 -0600 Subject: [PATCH 2/4] portforward: extract manual port forwarder to its own file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- peer/peer.go | 52 ++++++---------------------- peer/peer_test.go | 52 ---------------------------- portforward/manual.go | 71 ++++++++++++++++++++++++++++++++++++++ portforward/manual_test.go | 66 +++++++++++++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 93 deletions(-) create mode 100644 portforward/manual.go create mode 100644 portforward/manual_test.go diff --git a/peer/peer.go b/peer/peer.go index 0aec7b9e..cdde6f7d 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -7,7 +7,6 @@ import ( "fmt" "log/slog" "math/rand/v2" - "strconv" "sync" "sync/atomic" "time" @@ -22,41 +21,6 @@ import ( "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 { @@ -227,12 +191,18 @@ func NewClient(cfg Config) (*Client, error) { if port := uint16(settings.GetInt(settings.PeerManualPortKey)); port != 0 { slog.Info("peer client using manual port forward", "port", port, "source", "setting") - return &manualPortForwarder{port: port}, nil + return portforward.NewManualForwarder(port), nil } - if p := manualPort(); p != 0 { - slog.Info("peer client using manual port forward", - "port", p, "source", env.PeerExternalPort.String()) - return &manualPortForwarder{port: p}, 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) diff --git a/peer/peer_test.go b/peer/peer_test.go index eb392464..162ab7ad 100644 --- a/peer/peer_test.go +++ b/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") diff --git a/portforward/manual.go b/portforward/manual.go new file mode 100644 index 00000000..a8535b27 --- /dev/null +++ b/portforward/manual.go @@ -0,0 +1,71 @@ +package portforward + +import ( + "context" + "fmt" + "strconv" +) + +// ManualForwarder satisfies the portForwarder contract without talking +// to a UPnP gateway. 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 pointed +// peer.Client at it via setting or env var. +// +// Use case: networks where UPnP is disabled or unavailable (router has +// UPnP off for security, ISP-provided gateways without IGD, networks +// behind double-NAT). UPnP-based Forwarder fails in those environments. +type ManualForwarder struct { + port uint16 +} + +// NewManualForwarder builds a ManualForwarder for a pre-configured router +// port forward. port must be a valid TCP port; callers should obtain it +// from ParseManualPort (env-var path) or from a setting that already +// constrains the value to uint16. +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 +} diff --git a/portforward/manual_test.go b/portforward/manual_test.go new file mode 100644 index 00000000..68c9cfed --- /dev/null +++ b/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") +} From 217afc9785c2d209d5f4e11a3fd926a3a796dbea Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sat, 30 May 2026 14:40:13 -0600 Subject: [PATCH 3/4] peer/portforward/settings: address Copilot review on #500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- common/settings/settings.go | 14 ++++++++------ peer/peer.go | 22 ++++++++++++++++++---- portforward/manual.go | 7 +++---- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/common/settings/settings.go b/common/settings/settings.go index 1c5e5cb1..29c68d37 100644 --- a/common/settings/settings.go +++ b/common/settings/settings.go @@ -57,12 +57,14 @@ const ( 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 (single-port 1:1 NAT). When non-zero, - // peer.Client.Start uses portforward.ManualForwarder with this port - // instead of probing UPnP. 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 = use UPnP) + // 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] diff --git a/peer/peer.go b/peer/peer.go index cdde6f7d..9efab16e 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -188,10 +188,24 @@ func NewClient(cfg Config) (*Client, error) { // 2. RADIANCE_PEER_EXTERNAL_PORT env var (developer / // power-user override) // 3. fall through to UPnP discovery - if port := uint16(settings.GetInt(settings.PeerManualPortKey)); port != 0 { - slog.Info("peer client using manual port forward", - "port", port, "source", "setting") - return portforward.NewManualForwarder(port), nil + // + // 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) diff --git a/portforward/manual.go b/portforward/manual.go index a8535b27..8050d7d5 100644 --- a/portforward/manual.go +++ b/portforward/manual.go @@ -19,10 +19,9 @@ type ManualForwarder struct { port uint16 } -// NewManualForwarder builds a ManualForwarder for a pre-configured router -// port forward. port must be a valid TCP port; callers should obtain it -// from ParseManualPort (env-var path) or from a setting that already -// constrains the value to 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} } From 40f959d971040be50923ee8f1e15b7fed11043ea Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sat, 30 May 2026 14:49:34 -0600 Subject: [PATCH 4/4] peer/portforward: address Copilot review on #500 (round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- peer/peer.go | 5 ++++- portforward/manual.go | 13 +++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/peer/peer.go b/peer/peer.go index 9efab16e..0acf465f 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -184,11 +184,14 @@ func NewClient(cfg Config) (*Client, error) { // case every consumer router exposes). // // Resolution order: - // 1. settings.PeerManualPortKey (Advanced UI) + // 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 diff --git a/portforward/manual.go b/portforward/manual.go index 8050d7d5..a54ac3e7 100644 --- a/portforward/manual.go +++ b/portforward/manual.go @@ -6,15 +6,16 @@ import ( "strconv" ) -// ManualForwarder satisfies the portForwarder contract without talking -// to a UPnP gateway. 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 pointed -// peer.Client at it via setting or env var. +// 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). UPnP-based Forwarder fails in those environments. +// behind double-NAT). The UPnP-based Forwarder fails in those +// environments; this type lets callers bypass discovery entirely. type ManualForwarder struct { port uint16 }