Skip to content

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

Merged
myleshorton merged 4 commits into
fisk/peer-connection-events-Afrom
fisk/peer-manual-portforward
May 30, 2026
Merged

peer: manual port-forward override for UPnP-less networks#500
myleshorton merged 4 commits into
fisk/peer-connection-events-Afrom
fisk/peer-manual-portforward

Conversation

@myleshorton
Copy link
Copy Markdown
Contributor

@myleshorton myleshorton commented May 28, 2026

Summary

Two related changes to the manual-port-forward path that ship together:

  1. Consolidates the implementation into the portforward package. The manual forwarder landed in peer/peer.go via 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-based Forwarder — same interface, same package. This PR's first commit (22d1533) is a net-zero relocation: peer.manualPortForwarderportforward.ManualForwarder, peer.manualPortportforward.ParseManualPort, tests follow.

  2. Adds the PeerManualPortKey setting 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's NewForwarder:

  1. settings.PeerManualPortKey if non-zero (UI),
  2. else RADIANCE_PEER_EXTERNAL_PORT if set + parseable (developer override),
  3. else fall back to UPnP discovery (existing behavior).

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

  • Setting (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.
  • Env var (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 (full portForwarder contract).
  • peer/peer.go — drops manualPortForwarder + manualPort(); NewForwarder now calls portforward.NewManualForwarder / portforward.ParseManualPort. Drops the strconv import.
  • peer/peer_test.go — drops the now-moved tests.
  • ManualForwarder.MapPort reports "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 — adds PeerManualPortKey (int; 0 = use UPnP).
  • peer/peer.goNewForwarder resolution order now checks the setting first, then the env var, then falls through.

ManualForwarder contract

  • MapPort returns a Mapping with External == Internal == configured port and Method = "manual". No network roundtrip — the router rule is already in place.
  • UnmapPort is a no-op (user owns the router rule and removes it manually).
  • StartRenewal is a no-op (manually-configured rules don't carry a UPnP lease).
  • 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.

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 on peer/peer.go resolve 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.
  • Manual smoke (after build):
    • RADIANCE_PEER_EXTERNAL_PORT=5698 ./lantern-cli …peer.Client binds 5698, skips UPnP probe, log line peer client using manual port forward port=5698 source=RADIANCE_PEER_EXTERNAL_PORT.
    • From Flutter Advanced → manual port = 5698 → toggle SmC on → same outcome with source=setting.
    • Empty setting + missing env var → falls back to UPnP discovery (regression check).
    • Malformed env var (RADIANCE_PEER_EXTERNAL_PORT=abc) → warning logged, falls through to UPnP rather than failing Start.

Dependencies

🤖 Generated with Claude Code

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a manual external-port override for peer sharing so users on UPnP-less networks can still run “Share My Connection” by configuring a router port-forward themselves, with precedence: persisted setting → env var → UPnP discovery.

Changes:

  • Introduces portforward.ManualForwarder as a no-op forwarder that synthesizes a 1:1 mapping and returns empty ExternalIP for server-side observed-IP fallback.
  • Adds settings.PeerManualPortKey to persist a user-specified external TCP port.
  • Updates peer.Client forwarder selection to prefer the manual port (setting/env) before UPnP.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
portforward/manual.go Adds ManualForwarder + ParseManualPort to support manual port-forward setups.
portforward/manual_test.go Adds unit tests for ManualForwarder behavior and port parsing.
peer/peer.go Adds forwarder selection logic to use manual port configuration before UPnP.
common/settings/settings.go Adds PeerManualPortKey setting key and documentation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread portforward/manual.go Outdated
Comment thread portforward/manual.go Outdated
Comment thread portforward/manual.go Outdated
Comment thread portforward/manual_test.go Outdated
Comment thread common/settings/settings.go Outdated
Comment thread peer/peer.go Outdated
Comment thread peer/peer.go Outdated
@myleshorton myleshorton force-pushed the fisk/peer-connection-events-A branch from 4960eb5 to 5e36933 Compare May 29, 2026 02:02
@myleshorton myleshorton force-pushed the fisk/peer-manual-portforward branch from 3908a35 to df954ba Compare May 29, 2026 02:02
@myleshorton myleshorton force-pushed the fisk/peer-connection-events-A branch from 5e36933 to 854b2b1 Compare May 29, 2026 19:55
@myleshorton myleshorton force-pushed the fisk/peer-manual-portforward branch from df954ba to 33ca6ac Compare May 29, 2026 19:55
@myleshorton myleshorton force-pushed the fisk/peer-connection-events-A branch from 854b2b1 to c2bfbd2 Compare May 29, 2026 20:09
@myleshorton myleshorton force-pushed the fisk/peer-manual-portforward branch from 33ca6ac to c657273 Compare May 29, 2026 20:09
@myleshorton myleshorton force-pushed the fisk/peer-connection-events-A branch from c2bfbd2 to 915e95b Compare May 29, 2026 20:18
@myleshorton myleshorton force-pushed the fisk/peer-manual-portforward branch from c657273 to e77b4d9 Compare May 29, 2026 20:18
@myleshorton myleshorton force-pushed the fisk/peer-connection-events-A branch from 915e95b to 67311e7 Compare May 29, 2026 20:31
@myleshorton myleshorton force-pushed the fisk/peer-manual-portforward branch from e77b4d9 to 18f7db0 Compare May 29, 2026 20:32
@myleshorton myleshorton force-pushed the fisk/peer-connection-events-A branch from 67311e7 to 3ac9497 Compare May 29, 2026 20:45
@myleshorton myleshorton force-pushed the fisk/peer-manual-portforward branch from 18f7db0 to ad18425 Compare May 29, 2026 20:45
@myleshorton myleshorton force-pushed the fisk/peer-connection-events-A branch from 3ac9497 to 3debe39 Compare May 29, 2026 21:00
@myleshorton myleshorton force-pushed the fisk/peer-manual-portforward branch from ad18425 to 0c84c5a Compare May 29, 2026 21:04
Adam Fisk and others added 2 commits May 30, 2026 00:47
…_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>
@myleshorton myleshorton force-pushed the fisk/peer-manual-portforward branch from 22d1533 to 8667d26 Compare May 30, 2026 06:47
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>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread portforward/manual.go Outdated
Comment thread peer/peer.go
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>
@myleshorton myleshorton merged commit b2b3938 into fisk/peer-connection-events-A May 30, 2026
1 check passed
@myleshorton myleshorton deleted the fisk/peer-manual-portforward branch May 30, 2026 20:51
@myleshorton myleshorton restored the fisk/peer-manual-portforward branch May 30, 2026 21:02
myleshorton added a commit that referenced this pull request May 30, 2026
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>
myleshorton added a commit that referenced this pull request May 30, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants