diff --git a/common/settings/settings.go b/common/settings/settings.go index 3a3afd90..29c68d37 100644 --- a/common/settings/settings.go +++ b/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] diff --git a/peer/peer.go b/peer/peer.go index c9b87281..71f82b90 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" @@ -17,45 +16,11 @@ import ( box "github.com/getlantern/lantern-box" "github.com/getlantern/lantern-box/tracker/peerconn" "github.com/getlantern/radiance/common/env" + "github.com/getlantern/radiance/common/settings" "github.com/getlantern/radiance/events" "github.com/getlantern/radiance/portforward" ) -// manualPortForwarder satisfies the portForwarder interface without doing -// any UPnP work. Used when env.PeerExternalPort is set. -type manualPortForwarder struct{ port uint16 } - -func (m *manualPortForwarder) MapPort(_ context.Context, _ uint16, _ string) (*portforward.Mapping, error) { - return &portforward.Mapping{ - ExternalPort: m.port, - InternalPort: m.port, - Method: "manual-env", - }, nil -} -func (m *manualPortForwarder) UnmapPort(_ context.Context) error { return nil } -func (m *manualPortForwarder) StartRenewal(_ context.Context) {} -func (m *manualPortForwarder) ExternalIP(_ context.Context) (string, error) { - // An empty external IP signals the server to use the address it - // observed on the inbound request — when the user has supplied a - // manual port but no WAN IP, the server's view is the right answer. - return "", nil -} - -// manualPort returns the parsed env.PeerExternalPort value, or 0 if unset -// or invalid. -func manualPort() uint16 { - raw := env.GetString(env.PeerExternalPort) - if raw == "" { - return 0 - } - p, err := strconv.Atoi(raw) - if err != nil || p < 1 || p > 65535 { - slog.Warn("ignoring invalid "+env.PeerExternalPort.String(), "value", raw) - return 0 - } - return uint16(p) -} - // StatusEvent fires whenever the Client's session state changes — successful // Start, user Stop, or auto-Stop on a 404 heartbeat. type StatusEvent struct { @@ -209,10 +174,8 @@ func NewClient(cfg Config) (*Client, error) { } if cfg.NewForwarder == nil { cfg.NewForwarder = func(ctx context.Context) (portForwarder, error) { - if p := manualPort(); p != 0 { - slog.Info("peer client using manual port forward", - "port", p, "env", env.PeerExternalPort.String()) - return &manualPortForwarder{port: p}, nil + if fwd := pickManualForwarder(); fwd != nil { + return fwd, nil } // Explicitly return a nil interface on error — `return // portforward.NewForwarder(ctx)` collapses the (*Forwarder, error) @@ -647,6 +610,53 @@ func ensurePeerOutboundsBypassVPN(options string) (string, error) { return string(out), nil } +// pickManualForwarder resolves the manual port override against the +// two configured sources and returns a ManualForwarder, or nil if +// neither source supplies a valid port. The default NewForwarder +// factory in NewClient calls this first; nil means "fall through to +// UPnP discovery." +// +// Resolution order: +// +// 1. "peer_manual_port" setting (Advanced UI) +// 2. RADIANCE_PEER_EXTERNAL_PORT env var (developer / power-user) +// 3. nil — caller falls through to UPnP +// +// Persisted names are quoted so the comment stays accurate if Go +// identifiers move or rename. +// +// The setting is range-checked before casting to uint16 — a raw cast +// silently wraps negative values (-5 → 65531) and values above the +// port space (70000 → 4464), which would register a port the peer +// doesn't listen on (or, worse, one it does listen on for another +// service). Out-of-range / unparseable values are logged at Warn and +// the resolution falls through to the next source as if unset. +func pickManualForwarder() portForwarder { + if raw := settings.GetInt(settings.PeerManualPortKey); raw != 0 { + if raw < 1 || raw > 65535 { + slog.Warn("ignoring out-of-range peer_manual_port setting; falling through to env / UPnP", + "value", raw) + } else { + port := uint16(raw) + slog.Info("peer client using manual port forward", + "port", port, "source", "setting") + return portforward.NewManualForwarder(port) + } + } + if raw := env.GetString(env.PeerExternalPort); raw != "" { + port, err := portforward.ParseManualPort(raw) + if err != nil { + slog.Warn("ignoring invalid "+env.PeerExternalPort.String(), + "value", raw, "err", err) + } else { + slog.Info("peer client using manual port forward", + "port", port, "source", env.PeerExternalPort.String()) + return portforward.NewManualForwarder(port) + } + } + return nil +} + func pickInternalPort() uint16 { return uint16(internalPortMin + rand.IntN(internalPortMax-internalPortMin)) } diff --git a/peer/peer_test.go b/peer/peer_test.go index eb392464..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,58 +628,55 @@ func TestPickInternalPort_InRange(t *testing.T) { } } -// manualPort parses the RADIANCE_PEER_EXTERNAL_PORT env var. Unset, empty, -// non-numeric, and out-of-range values all collapse to 0, which the -// NewClient default factory treats as "no override → use UPnP discovery". -// Only a 1..65535 value selects the manual path. -func TestManualPort(t *testing.T) { +// pickManualForwarder is the default-NewForwarder factory's first +// branch: setting → env-var → nil (= caller falls through to UPnP). +// Tests each resolution path and the out-of-range / unparseable +// fallthrough behavior. Out-of-range setting + unset env returns nil +// — peer.NewClient's caller treats that as "use UPnP discovery." +func TestPickManualForwarder(t *testing.T) { tests := []struct { - name string - env string - want uint16 + name string + setting int // 0 means unset + envVar string // "" means unset + wantManual bool + wantPort uint16 }{ - {"unset", "", 0}, - {"valid mid-range", "5698", 5698}, - {"valid low boundary", "1", 1}, - {"valid high boundary", "65535", 65535}, - {"non-numeric", "abc", 0}, - {"zero", "0", 0}, - {"negative", "-5", 0}, - {"above uint16", "65536", 0}, - {"way above uint16", "99999", 0}, + {"setting takes precedence over env", 5698, "1234", true, 5698}, + {"setting only", 5698, "", true, 5698}, + {"env only", 0, "5698", true, 5698}, + {"both unset → fall through", 0, "", false, 0}, + {"setting out of range, env unset → fall through", 70000, "", false, 0}, + {"setting negative, env unset → fall through", -5, "", false, 0}, + {"setting out of range, env valid → env wins", 70000, "5698", true, 5698}, + {"setting unset, env unparseable → fall through", 0, "abc", false, 0}, + {"setting valid low boundary", 1, "", true, 1}, + {"setting valid high boundary", 65535, "", true, 65535}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - t.Setenv("RADIANCE_PEER_EXTERNAL_PORT", tc.env) - assert.Equal(t, tc.want, manualPort()) + require.NoError(t, settings.InitSettings(t.TempDir())) + t.Cleanup(settings.Reset) + if tc.setting != 0 { + require.NoError(t, settings.Set(settings.PeerManualPortKey, tc.setting)) + } + t.Setenv("RADIANCE_PEER_EXTERNAL_PORT", tc.envVar) + + fwd := pickManualForwarder() + if !tc.wantManual { + assert.Nil(t, fwd, "expected fall-through (nil) but got a forwarder") + return + } + require.NotNil(t, fwd, "expected manual forwarder, got nil") + // Verify the chosen port via the public MapPort surface — + // ManualForwarder.port is unexported, MapPort echoes it. + mapping, err := fwd.MapPort(context.Background(), 0, "") + require.NoError(t, err) + assert.Equal(t, tc.wantPort, mapping.ExternalPort) + assert.Equal(t, tc.wantPort, mapping.InternalPort) }) } } -// manualPortForwarder must satisfy the portForwarder contract: MapPort -// returns a Mapping using the configured port for both internal and -// external (no rewrite — that's the user's responsibility), UnmapPort -// and StartRenewal are no-ops, and ExternalIP returns "" so the server -// substitutes the IP it observed on the request. -func TestManualPortForwarder(t *testing.T) { - f := &manualPortForwarder{port: 5698} - - m, err := f.MapPort(context.Background(), 30001, "ignored") - require.NoError(t, err) - assert.Equal(t, uint16(5698), m.ExternalPort) - assert.Equal(t, uint16(5698), m.InternalPort, "external==internal — user mapped them themselves") - assert.Equal(t, "manual-env", m.Method) - - require.NoError(t, f.UnmapPort(context.Background()), "UnmapPort is a no-op for manual forwarders") - - // StartRenewal must not panic or block. - f.StartRenewal(context.Background()) - - ip, err := f.ExternalIP(context.Background()) - require.NoError(t, err) - assert.Empty(t, ip, "empty ip signals server to use observed source address") -} - func TestAPIError_StringFormat(t *testing.T) { e := &APIError{Status: 422, Body: "could not connect to peer port"} assert.Contains(t, e.Error(), "422") diff --git a/portforward/manual.go b/portforward/manual.go new file mode 100644 index 00000000..977c9393 --- /dev/null +++ b/portforward/manual.go @@ -0,0 +1,84 @@ +package portforward + +import ( + "context" + "fmt" + "strconv" +) + +// ManualForwarder exposes the same Map/Unmap/StartRenewal/ExternalIP +// surface as Forwarder but does no UPnP work. The user is expected to +// have configured a port forward on their router by hand and supplied +// the port number out-of-band. This implementation reports the same +// value for both the external and internal port — callers needing +// distinct external/internal ports should use the UPnP-based Forwarder +// (which can negotiate them) or build their own portForwarder. +// +// Use case: networks where UPnP is disabled or unavailable (router has +// UPnP off for security, ISP-provided gateways without IGD, networks +// behind double-NAT). The UPnP-based Forwarder fails in those +// environments; this type lets callers bypass discovery entirely. +type ManualForwarder struct { + port uint16 +} + +// NewManualForwarder builds a ManualForwarder for a pre-configured +// router port forward. port must be in 1..65535; the caller is +// responsible for validating its input before calling. +func NewManualForwarder(port uint16) *ManualForwarder { + return &ManualForwarder{port: port} +} + +// ParseManualPort parses a string into a TCP port number. Values outside +// 1..65535 return an error so callers can log and fall through to UPnP +// discovery rather than register a non-listening port with the server. +func ParseManualPort(s string) (uint16, error) { + p, err := strconv.Atoi(s) + if err != nil { + return 0, fmt.Errorf("parse %q: %w", s, err) + } + if p < 1 || p > 65535 { + return 0, fmt.Errorf("port %d out of range (1..65535)", p) + } + return uint16(p), nil +} + +// MapPort reports the configured port as both external and internal. The +// router-side rule is already in place; nothing to do at the protocol +// layer. Returns an error if the forwarder was constructed with port==0 +// (not a valid listen/registration port) so callers can fall through to +// UPnP rather than register a port no peer will listen on. +// +// Protocol is set to "TCP" to match the UPnP-based Forwarder, which +// hard-codes the same value. Samizdat-in inbound traffic is TCP-only; +// when UDP support is added the two forwarders should be widened +// together. +func (m *ManualForwarder) MapPort(_ context.Context, _ uint16, _ string) (*Mapping, error) { + if m.port == 0 { + return nil, fmt.Errorf("manual forwarder constructed with port=0") + } + return &Mapping{ + ExternalPort: m.port, + InternalPort: m.port, + Protocol: "TCP", + Method: "manual", + }, nil +} + +// UnmapPort is a no-op: the user owns the router rule and is responsible +// for removing it. +func (m *ManualForwarder) UnmapPort(_ context.Context) error { return nil } + +// StartRenewal is a no-op: manually-configured rules don't carry a UPnP +// lease and don't need refreshing. +func (m *ManualForwarder) StartRenewal(_ context.Context) {} + +// ExternalIP returns the empty string deliberately. With a manual port +// forward we have no UPnP gateway to ask for the WAN address, and +// probing a public IP service from the client adds a network roundtrip +// for information lantern-cloud already has — the server observes the +// peer's source address on the register call and uses that as the +// canonical external IP when this field is empty. +func (m *ManualForwarder) ExternalIP(_ context.Context) (string, error) { + return "", nil +} diff --git a/portforward/manual_test.go b/portforward/manual_test.go new file mode 100644 index 00000000..49d01253 --- /dev/null +++ b/portforward/manual_test.go @@ -0,0 +1,78 @@ +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, 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) + + 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, "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") + + // 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") +} + +// 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) +} 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) {