peer: manual port-forward override for UPnP-less networks#503
Open
myleshorton wants to merge 7 commits into
Open
peer: manual port-forward override for UPnP-less networks#503myleshorton wants to merge 7 commits into
myleshorton wants to merge 7 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates the “manual port-forward” implementation into the portforward package and extends the peer client’s forwarder selection to support a persisted manual-port setting (for UPnP-less networks) ahead of the existing env-var override, falling back to UPnP discovery otherwise.
Changes:
- Adds
portforward.ManualForwarderandportforward.ParseManualPort, along with focused unit tests. - Updates
peer.ClientdefaultNewForwarderto resolve manual port configuration viasettings.PeerManualPortKey→RADIANCE_PEER_EXTERNAL_PORT→ UPnP. - Introduces the
PeerManualPortKeypersisted setting (peer_manual_port) incommon/settings.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
portforward/manual.go |
New manual forwarder implementation and parsing helper for manual port overrides. |
portforward/manual_test.go |
New tests for manual port parsing and manual forwarder behavior. |
peer/peer.go |
Removes in-file manual forwarder implementation; uses portforward package and adds setting-first resolution order. |
peer/peer_test.go |
Removes tests that were tied to the old in-file manual forwarder implementation. |
common/settings/settings.go |
Adds PeerManualPortKey setting constant and documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
myleshorton
added a commit
that referenced
this pull request
May 30, 2026
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 <noreply@anthropic.com>
…_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>
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>
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>
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>
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 <noreply@anthropic.com>
myleshorton
force-pushed
the
fisk/peer-manual-portforward
branch
from
907c9dc to
7f8224d
Compare
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 <noreply@anthropic.com>
…check 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related changes to the manual-port-forward path that ship together:
Consolidates the implementation into the
portforwardpackage. The manual forwarder landed inpeer/peer.govia peer: call /peer/verify after starting sing-box; fix doubled /v1 #466 to keep that PR's review surface small, but architecturally it belongs alongside the UPnP-basedForwarder— same interface, same package. This PR's first commit (22d1533) is a net-zero relocation:peer.manualPortForwarder→portforward.ManualForwarder,peer.manualPort→portforward.ParseManualPort, tests follow.Adds the
PeerManualPortKeysetting so the Advanced UI in the Share My Connection screen can configure the port without an env var. The env-var path stays for developer / power-user use.After both commits, the resolution order in
peer.Client.Start'sNewForwarder:settings.PeerManualPortKeyif non-zero (UI),RADIANCE_PEER_EXTERNAL_PORTif set + parseable (developer override),A non-positive or malformed env-var value is logged and the resolution falls through to UPnP discovery rather than registering a non-listening port with lantern-cloud.
Why two configuration paths
PeerManualPortKey) — for users on networks where UPnP is unavailable (ISP gateways with UPnP off, double-NAT, residential routers with the feature disabled). The Advanced section in the Share My Connection Flutter screen sets it; persists across restarts. Cleared by setting the value to 0.RADIANCE_PEER_EXTERNAL_PORT) — developer override for testing on macOS / CLI without a UI build. Same parser, same forwarder.What's in this PR
Consolidation (commit
22d1533):portforward/manual.go(new) —ManualForwarder,NewManualForwarder(port uint16),ParseManualPort(s string) (uint16, error).portforward/manual_test.go(new) —TestParseManualPort(9 input cases including boundaries + invalid),TestManualForwarder(fullportForwardercontract).peer/peer.go— dropsmanualPortForwarder+manualPort();NewForwardernow callsportforward.NewManualForwarder/portforward.ParseManualPort. Drops thestrconvimport.peer/peer_test.go— drops the now-moved tests.ManualForwarder.MapPortreports"manual"as the method tag (was"manual-env"when it only served the env path; the suffix is wrong now that both paths feed in).Setting integration (commit
0c84c5a):common/settings/settings.go— addsPeerManualPortKey(int; 0 = use UPnP).peer/peer.go—NewForwarderresolution order now checks the setting first, then the env var, then falls through.ManualForwardercontractMapPortreturns aMappingwithExternal == Internal == configured portandMethod = "manual". No network roundtrip — the router rule is already in place.UnmapPortis a no-op (user owns the router rule and removes it manually).StartRenewalis a no-op (manually-configured rules don't carry a UPnP lease).ExternalIPreturns 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.How this was sliced
Stacked on radiance #499 (
fisk/peer-connection-events-A), itself stacked on #466 → #460 → #458. After cascade rebases through the stack, conflicts onpeer/peer.goresolve cleanly — the consolidation commit moves code that other commits in the chain don't touch.Test plan
go test -race -count=1 ./peer/... ./portforward/... ./backend/...clean on the branch.RADIANCE_PEER_EXTERNAL_PORT=5698 ./lantern-cli …→peer.Clientbinds 5698, skips UPnP probe, log linepeer client using manual port forward port=5698 source=RADIANCE_PEER_EXTERNAL_PORT.source=setting.RADIANCE_PEER_EXTERNAL_PORT=abc) → warning logged, falls through to UPnP rather than failing Start.Dependencies
fisk/peer-connection-events-A) — this PR's base.🤖 Generated with Claude Code