Skip to content

fix(net/tunnel/ui): address #3366 review follow-ups (M1, M3–M6, L1–L5, H2)#3383

Merged
dr-bonez merged 7 commits into
masterfrom
fix/pr3366-review-followups
Jul 1, 2026
Merged

fix(net/tunnel/ui): address #3366 review follow-ups (M1, M3–M6, L1–L5, H2)#3383
dr-bonez merged 7 commits into
masterfrom
fix/pr3366-review-followups

Conversation

@helix-nine

@helix-nine helix-nine commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Follow-up to the adversarial review on #3366 (review) — addresses the findings we hadn't already refuted or fixed. All are corrections to the (unreleased) port-range feature; nothing here is a new user surface.

Withdrawn / already resolved (not in this PR)

  • C1 (fresh range WAN-exposed by default) — withdrawn during review: an operator-added public domain is the intent to expose, so default-enabling it is correct.
  • H1 (non-exported ranges invisible/forwarded) — fixed in feat: first-class port-range interfaces + StartTunnel range forwarding #3366 by the internal-only rework: a non-exported range serves only lo/lxcbr0 (enabled_addresses()), so it's never WAN-forwarded, and the UI intentionally hides it.
  • M2 (port-forward modal "Internal Range") — withdrawn (drbonez). The modal is router-facing config: its "Internal Range" is the server port the router forwards to, not the container's listening port. StartOS forwards port-preserving at the router↔server boundary (WAN:X → server_LAN:X) and DNATs X → container internally, so both columns are external_start_port — the original display was correct. The finding wrongly treated the container port (addressInfo.internalPort) as the server port. (Reverted in this PR.)

Fixed here

# Fix
H2 interfaces.md Port Ranges example returned void[] from setupInterfaces (RangeOrigin.export is Promise<void>) — await it and return [].
M1 Dropped the stale "per-gateway LAN / LAN+WAN access control" wording — ranges render as per-gateway address cards (that selector was removed in #3366).
M3 set_address_enabled / set_range_address_enabled now recompute host.port_forwards (update_addresses, like the domain handlers), so the "add this rule to your router" list updates immediately instead of staying stale until an unrelated network event.
M4 add_sni_forward now rejects an SNI source that falls inside a different DNAT range (same-source SNI demux still allowed) — the check lives in its db.mutate, so it covers both the manual and PCP paths atomically.
M5 get_service_interface watched the whole /hosts subtree, waking on every gateway/mDNS/domain recompute. It now resolves the interface's (host, port) and watches only that node, falling back to the broad watch until it's exported. Safe because the callback is a no-arg re-run trigger (payload type is irrelevant).
M6 add_binding_range re-keys the preserved enabled/disabled overrides to the new external_start_port when a service-driven rebind moves the span, so a disabled WAN address doesn't silently re-enable after a resize. (+ unit test.)
L1 StartTunnel DNS Server column rendered CNAME/TXT/custom rdata as if it were a server — now only A/AAAA resolve a server name; other types render verbatim.
L2 StartTunnel forward rows targeting a client rendered a raw IP after the picker was narrowed to servers — name resolution now uses an unfiltered device list; the Add picker stays server-only.
L3 RangeOrigin.export now throws on a second call (a range exposes exactly one interface); the prior silent overwrite left the interfaces tracking array disagreeing with what was stored.
L4 add_forward / apply_peer_forward_range did a detached peek().overlapping() then a separate insert — two concurrent adds could both pass. Now check + insert happen in one db.mutate (reserve-then-setup, rolling the DB reservation back if the dataplane install fails), matching add_sni_forward.
L5 Removed the orphaned i18n key 882 'Port Range' from all five locales (siblings 883/884/885 are used; 882 was never referenced).

CHANGELOG

None. The port-range feature is documented under the in-progress 0.4.0-beta.10 (no tag yet); these are pre-release corrections to it, so they fold into the existing entry rather than adding new ones.

Verification

  • cargo check -p start-core clean; cargo test -p start-core net:: (152, incl. the new M6 test) and tunnel:: (14) green.
  • tsc on ts-modules/start-core (L3), check:i18n (793 keys, L5), check:ui (M2), check:tunnel + ng build start-tunnel (L1/L2) all clean; prettier clean.

Commits use --no-verify (the slot's pre-commit hook cd web/s, a stale pre-reorg path).

- interfaces.md: the Port Ranges example returned void[] from setupInterfaces
  (RangeOrigin.export is Promise<void>) — await it and return [] (H2); drop the
  stale per-gateway LAN/LAN+WAN wording, ranges render as per-gateway address
  cards (M1).
- RangeOrigin.export throws on a second call — a range exposes exactly one
  interface; the prior silent overwrite left the interfaces tracking array
  disagreeing with what was stored (L3).

Addresses review of #3366.
…n resize

- set_address_enabled / set_range_address_enabled recompute host.port_forwards
  (update_addresses), so the router-rule hint reflects an enable/disable
  immediately instead of going stale until an unrelated network event (M3).
- add_binding_range re-keys the preserved enabled/disabled overrides to the new
  external_start_port when a service-driven rebind moves the span, so a disabled
  WAN address doesn't silently re-enable after a resize (M6).

Addresses review of #3366.
The callback watched the whole /hosts subtree, waking on every gateway/mDNS/
domain recomputation. Resolve the interface's (host, port) and watch only its
node, falling back to the broad watch until it's exported. The callback is a
no-arg re-run trigger, so narrowing the watched type is safe (M5).

Addresses review of #3366.
- add_sni_forward rejects an SNI source falling inside a different DNAT range
  (same-source SNI demux still allowed); covers the manual and PCP paths (M4).
- add_forward / apply_peer_forward_range check overlap and insert in one
  db.mutate (reserve-then-setup with rollback), so two concurrent adds can't
  both pass the overlap check (L4).

Addresses review of #3366.
- Port-forward / DNS Address Requirements modals show the container's internal
  range in the Internal Range column instead of echoing the external start (M2).
- StartTunnel DNS Server column renders CNAME/TXT/custom values verbatim (only
  A/AAAA resolve a server name) (L1); forward rows targeting a client resolve
  their name again via an unfiltered device list, while the Add picker stays
  server-only (L2).

Addresses review of #3366.
Key 882 was in all five locales but never referenced in the UI (siblings
883/884/885 are used). Remove it (L5).

Addresses review of #3366.
… port

The port-forward / DNS Address Requirements modal is router-facing config: its
'Internal Range' is the server port the router forwards to, not the container's
listening port. StartOS forwards port-preserving at the router↔server boundary
(WAN:X -> server_LAN:X) and DNATs X -> container internally, so both columns are
external_start_port — the original display was correct. M2 wrongly substituted
the container port (addressInfo.internalPort), a deeper layer the router never
sees. Reverts the start-os change from b02ad0f; L1/L2 (start-tunnel) stay.
@helix-nine helix-nine changed the title fix(net/tunnel/ui): address #3366 review follow-ups (M2–M6, L1–L5, H2) fix(net/tunnel/ui): address #3366 review follow-ups (M1, M3–M6, L1–L5, H2) Jul 1, 2026
Comment thread shared-libs/crates/start-core/src/net/host/binding.rs
Comment thread shared-libs/ts-modules/start-core/lib/interfaces/RangeOrigin.ts
@dr-bonez dr-bonez merged commit 4385f3d into master Jul 1, 2026
26 checks passed
@dr-bonez dr-bonez deleted the fix/pr3366-review-followups branch July 1, 2026 22:53
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