From 0e1470d8b7a7a8c986ed38a879f664980e400d69 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Thu, 7 May 2026 14:28:47 -0600 Subject: [PATCH 1/7] 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 b49872c1560d172e139c46554b63a06c540aea6d Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Fri, 29 May 2026 15:14:28 -0600 Subject: [PATCH 2/7] 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 d460569e192a69c73853eca9fce87bd705eab304 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sat, 30 May 2026 14:40:13 -0600 Subject: [PATCH 3/7] 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 d517ef8ca8c6e5d1b389eaaadae16347fd44f5bd Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sat, 30 May 2026 14:49:34 -0600 Subject: [PATCH 4/7] 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 } From 7f8224da7eaea5b4e74f87da659004552bbdc668 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sat, 30 May 2026 15:16:40 -0600 Subject: [PATCH 5/7] peer/portforward: address Copilot review on #503 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings (each surfaced as a duplicate thread, so 6 total): 1. portforward.ManualForwarder doc claimed 'every consumer router exposes port forwarding as a single port number' — broad and inaccurate (many routers support distinct external/internal ports). Reworded as an implementation requirement: 'this implementation reports the same value for both the external and internal port — callers needing distinct ports should use the UPnP-based Forwarder.' 2. Same misleading claim was inline in peer.go's NewForwarder closure comment. Replaced with the same implementation-requirement framing. 3. The manual-port resolution order (setting → env → UPnP) and the out-of-range setting behavior had no test coverage. Extracted the resolution logic into pickManualForwarder() so it's directly testable without standing up a real UPnP probe. The default NewForwarder factory now calls pickManualForwarder() first; nil return means fall through to UPnP. New TestPickManualForwarder covers 10 cases: - setting takes precedence over env - setting-only / env-only / both-unset - setting out-of-range (positive + negative) → fallthrough - setting out-of-range + env valid → env wins - setting unset + env unparseable → fallthrough - low/high boundary values (1 and 65535) No behavior change — the extraction is line-for-line equivalent to the previous inline logic. Co-Authored-By: Claude Opus 4.7 --- peer/peer.go | 95 ++++++++++++++++++++++--------------------- peer/peer_test.go | 50 +++++++++++++++++++++++ portforward/manual.go | 8 ++-- 3 files changed, 104 insertions(+), 49 deletions(-) diff --git a/peer/peer.go b/peer/peer.go index 0acf465f..ad51b21d 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -174,52 +174,8 @@ 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. "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 - } + if fwd := pickManualForwarder(); fwd != nil { + return fwd, nil } // Explicitly return a nil interface on error — `return // portforward.NewForwarder(ctx)` collapses the (*Forwarder, error) @@ -654,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, "error", 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)) } diff --git a/peer/peer_test.go b/peer/peer_test.go index 162ab7ad..7d41b8ef 100644 --- a/peer/peer_test.go +++ b/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,6 +628,55 @@ func TestPickInternalPort_InRange(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 + setting int // 0 means unset + envVar string // "" means unset + wantManual bool + wantPort uint16 + }{ + {"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) { + 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) + }) + } +} + 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 index a54ac3e7..8997cc23 100644 --- a/portforward/manual.go +++ b/portforward/manual.go @@ -8,9 +8,11 @@ import ( // 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. +// 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 From eb55d0fd29fb4c876813835092bf7c6ba023043a Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sat, 30 May 2026 16:11:03 -0600 Subject: [PATCH 6/7] peer/portforward: address Copilot review on #503 (round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit portforward/manual.go: - ManualForwarder.MapPort now sets Protocol="TCP" to match the UPnP-based Forwarder, which hard-codes the same value. Samizdat-in inbound traffic is TCP-only on both code paths; widening to UDP later means widening both forwarders together. - Defensive guard: MapPort returns an error when constructed with port==0. pickManualForwarder already range-checks 1..65535 before calling NewManualForwarder, but the check belongs on the type itself — a caller that bypasses the validator (programmatic use, tests, future code paths) gets a clear error instead of silently registering port 0 with lantern-cloud. portforward/manual_test.go: - Asserts Protocol="TCP" in TestManualForwarder. - TestManualForwarder_RejectsZeroPort verifies the new guard. peer/peer.go: - slog warning in the env-var path used 'error' as the attribute key while every other log line in this file uses 'err'. Renamed for log-aggregation consistency. Co-Authored-By: Claude Opus 4.7 --- peer/peer.go | 2 +- portforward/manual.go | 13 ++++++++++++- portforward/manual_test.go | 18 +++++++++++++++--- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/peer/peer.go b/peer/peer.go index ad51b21d..71f82b90 100644 --- a/peer/peer.go +++ b/peer/peer.go @@ -647,7 +647,7 @@ func pickManualForwarder() portForwarder { port, err := portforward.ParseManualPort(raw) if err != nil { slog.Warn("ignoring invalid "+env.PeerExternalPort.String(), - "value", raw, "error", err) + "value", raw, "err", err) } else { slog.Info("peer client using manual port forward", "port", port, "source", env.PeerExternalPort.String()) diff --git a/portforward/manual.go b/portforward/manual.go index 8997cc23..977c9393 100644 --- a/portforward/manual.go +++ b/portforward/manual.go @@ -45,11 +45,22 @@ func ParseManualPort(s string) (uint16, error) { // 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. +// 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 } diff --git a/portforward/manual_test.go b/portforward/manual_test.go index 68c9cfed..49d01253 100644 --- a/portforward/manual_test.go +++ b/portforward/manual_test.go @@ -43,9 +43,10 @@ func TestParseManualPort(t *testing.T) { } // 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. +// a Mapping with external==internal port, Protocol="TCP" matching the +// UPnP forwarder, 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) @@ -53,6 +54,7 @@ func TestManualForwarder(t *testing.T) { 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, "TCP", m.Protocol, "Protocol matches UPnP forwarder's hard-coded value") assert.Equal(t, "manual", m.Method) require.NoError(t, f.UnmapPort(context.Background()), "UnmapPort is a no-op for manual forwarders") @@ -64,3 +66,13 @@ func TestManualForwarder(t *testing.T) { require.NoError(t, err) assert.Empty(t, ip, "empty IP signals server to use observed source address") } + +// MapPort defensively rejects a zero-port forwarder so a caller that +// somehow gets one (bypassing pickManualForwarder's range check) can +// fall through to UPnP rather than register a non-listening port. +func TestManualForwarder_RejectsZeroPort(t *testing.T) { + f := NewManualForwarder(0) + m, err := f.MapPort(context.Background(), 30001, "ignored") + assert.Nil(t, m) + assert.Error(t, err) +} From 79400efec30e90a373de9b300461f11841ce72b9 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sun, 31 May 2026 16:13:44 -0600 Subject: [PATCH 7/7] portforward: add ProbeUPnP for the pre-flight 'would SmC work here?' check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Share My Connection UI flow needs to decide which mode to start (Full SmC vs Unbounded) based on whether UPnP discovery succeeds on the user's network. Previously that gate didn't exist in the lib — share_my_connection.dart was using Random().nextBool() as a stand-in, which routed half of opted-in users to a mode that didn't match their actual capabilities. ProbeUPnP wraps NewForwarder and returns true on success, false on any failure (ErrNoPortForwarding, ctx timeout, ctx cancellation). The discovered forwarder is discarded after the probe; Forwarder holds no goroutines or sockets that need explicit cleanup, so a subsequent NewForwarder call can re-discover without coordination. Callers (the lantern-core FFI export + the Dart-side isPeerProxyEnabled-style call) treat true/false binary; the underlying error is not surfaced because no UI flow does anything productive with the distinction between 'no IGD on this LAN' and 'discovery timed out'. TestProbeUPnP_CancelledContextReturnsFalse pins the cancellation-fast-path contract: a cancelled ctx must yield false within ~2s, not block for the M-SEARCH multicast wait. A positive- path test would require a real IGD on the CI host's network, which isn't available. Co-Authored-By: Claude Opus 4.7 --- portforward/portforward.go | 22 ++++++++++++++++++++++ portforward/portforward_test.go | 18 ++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/portforward/portforward.go b/portforward/portforward.go index 576f7e38..3ae2ff93 100644 --- a/portforward/portforward.go +++ b/portforward/portforward.go @@ -50,6 +50,28 @@ type Forwarder struct { cancel context.CancelFunc } +// ProbeUPnP reports whether IGD discovery on the local network turns up a +// gateway that could host a port mapping. No port is actually mapped — this +// is the "would Share My Connection's residential-proxy mode work on this +// network?" pre-flight check that UI flows show before committing to the +// long-lived SmC path. +// +// True means discovery succeeded; false means either no gateway was found +// (ErrNoPortForwarding) or ctx expired before discovery completed. Callers +// generally treat both as "not available" — distinguishing them requires +// returning the underlying error, which most UI surfaces don't have a +// productive use for. +// +// Pick a 5-10s timeout on ctx: M-SEARCH multicast waits for replies and a +// fast bail leaves slower gateways unmatched. The discovered forwarder is +// discarded after the probe; it holds no goroutines or sockets that need +// explicit cleanup, so a subsequent NewForwarder call can re-discover +// without coordination. +func ProbeUPnP(ctx context.Context) bool { + _, err := NewForwarder(ctx) + return err == nil +} + // NewForwarder discovers the local gateway and returns a Forwarder bound to // it. Callers should pick a 5-10s timeout on ctx — UPnP discovery is M-SEARCH // multicast and waits for replies. diff --git a/portforward/portforward_test.go b/portforward/portforward_test.go index 7d6e0ee2..c52982b5 100644 --- a/portforward/portforward_test.go +++ b/portforward/portforward_test.go @@ -107,6 +107,24 @@ func TestForwarder_MapPort_PropagatesGatewayError(t *testing.T) { assert.ErrorContains(t, err, "add port mapping") } +// ProbeUPnP wraps NewForwarder and returns false on any error, including +// ctx cancellation / deadline expiration. A successful probe requires a +// real IGD on the test host's network, which CI doesn't have — but the +// negative-path contract (cancelled ctx → false within the cancel +// window, no leaked goroutines) is what callers actually depend on for +// timely UI feedback when UPnP is unavailable. +func TestProbeUPnP_CancelledContextReturnsFalse(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + start := time.Now() + got := ProbeUPnP(ctx) + elapsed := time.Since(start) + + assert.False(t, got, "cancelled ctx must yield false") + assert.Less(t, elapsed, 2*time.Second, "probe should bail fast on a cancelled ctx, not wait for M-SEARCH") +} + // MapPort must respect the caller's context — a hung router shouldn't tie up // Start past its deadline. func TestForwarder_MapPort_RespectsContextCancellation(t *testing.T) {