Skip to content

feat: first-class port-range interfaces + StartTunnel range forwarding#3366

Merged
dr-bonez merged 8 commits into
masterfrom
feat/port-range-interfaces
Jul 1, 2026
Merged

feat: first-class port-range interfaces + StartTunnel range forwarding#3366
dr-bonez merged 8 commits into
masterfrom
feat/port-range-interfaces

Conversation

@MattDHill

Copy link
Copy Markdown
Member

Makes port ranges first-class service interfaces in StartOS, adds port-range forwarding to StartTunnel, and overhauls the StartTunnel UI. Also folds in a repo-reference cleanup.

Port ranges as first-class interfaces (StartOS)

Previously a port range was a bespoke binding with a tri-state gateway flag and no address data. This reworks it so a port-range interface behaves almost exactly like a single-port interface.

  • SDKbindPortRange returns a RangeOrigin that exports exactly one restricted api interface (no type/masked/username/path/query). New RangeInterfaceBuilder / RangeOrigin; packaging docs updated.
  • Backend addressing — a range now carries a full DerivedAddressInfo (mDNS / LAN IPv4 / WAN IPv4 / public+private domains), IPv4-only and non-SSL, synthesized using its external_start_port as the representative port so the entire single-port machinery (enable/disable, forward derivation, domain add/remove) is reused. The tri-state RangeGatewayAccess enum and its RPC are removed.
  • Interface lookup — dropped serviceInterface.getOwn/get; added host.get/getOwn (walk host → bindings/binding-ranges → interfaces). PackageDataEntry.serviceInterfaces is gone — interfaces are derived from host → binding → interface.
  • UI — the separate Port-Ranges dashboard tab is removed; ranges render in the service Interfaces page as per-gateway address cards identical to single-port, each address toggleable, with Add-Domain. The deprecated .startos internal addressing is never shown. The Certificate Authority column only appears when an address is actually SSL; a range's URL is the bare host (the span shows once in the header); the Address-Requirements port-forwarding section shows the port range (not a single port) and drops the Test button for ranges, since a range can't be probed a port at a time.

StartTunnel: port-range forwarding

The automatic PCP PORT_SET path already forwarded ranges; this completes the manual API and UI, and hardens both paths.

  • API (tunnel/api.rs) — AddPortForwardParams gains an optional count (#[ts(optional)], wire-compatible). add_forward validates it, rejects SNI + range, guards the u16 span, calls add_forward_range, and stores the real count.
  • Overlap detection — new shared PortForwards::overlapping(source, count) (a DNAT spans its count, an SNI forward holds one port) is enforced on both the manual API and the automatic PCP/UPnP path (apply_peer_forward_range), so two different start ports can no longer cover overlapping external ports. Unit-tested.
  • UI — the Add dialog gets a "Number of Ports" field (client-side overflow validation, SNI hidden for ranges); the forward tables render a range as start-end.
  • Bindings regenerated; start-tunnel bumped to 1.1.0 with a changelog entry; port-forward add manpage + cli-reference + port-forwarding docs updated for --count.

StartTunnel UI/UX

  • Forwards & DNS target servers only. Port forwards and DNS records can only point at servers, so both forms now filter the picker to server-kind devices; the Port-Forwards "Device" column/field is now Server, and the DNS "Value" column/field is Server — rendering server name (IP) and dropping the now-redundant "Source" column.
  • Device role action relabeled from "Promote/Demote" to "Change to Server/Client" (with server/client icons), docs updated to match.
  • Card-header icons added (Subnets, Servers, Clients, Manual, Automatic).
  • Bordered tables matching the StartOS UI — rounded, single-line row separators (border-collapse: separate + overflow: hidden).
  • Removed the redundant page subheader that just mirrored the selected sidebar tab, and restored the top inset so cards don't butt the top bar.
  • Toggle loaders now hug their switch; the Automatic tables drop the ⋮ overflow (delete doesn't apply to auto entries) and left-align their last column.

Repo-reference cleanup

Updated stale Start9Labs/start-os references (the repo was renamed to start-technologies) across the five monorepo product crates, the vendored Start9 crates (patch-db, rpc-toolkit, exver, pi-beep, yasi), CI/release workflows, package metadata, and docs. External service repos, third-party forks, and Start9 fork git-dependencies are intentionally left untouched.

🤖 Generated with Claude Code

https://claude.ai/code/session_01L5wgrjn2CT7ASgqeJv5s1B

- StartOS: bindPortRange exports one restricted `api` interface; ranges render
  in the Interfaces page as per-gateway address cards (no tri-state). Drop
  PackageDataEntry.serviceInterfaces and serviceInterface.get/getOwn for
  host-derived interfaces and host.get/getOwn.
- StartTunnel: complete the manual port-forward API + UI for ranges and add
  overlap detection across the manual and automatic forward paths.
- StartTunnel UI: forwards/DNS target servers only (Device/Value -> Server),
  card-header icons, bordered tables, drop the redundant page subheader.
- Update stale Start9Labs/start-os references to start-technologies.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L5wgrjn2CT7ASgqeJv5s1B
@MattDHill MattDHill requested review from dr-bonez and waterplea June 30, 2026 04:55

@helix-nine helix-nine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the full diff and ran what's testable locally. Overall this is a clean, well-structured refactor — collapsing the bespoke RangeGatewayAccess tri-state into the same DerivedAddressInfo machinery that single-port bindings use is the right call, and moving service interfaces out of the flat PackageDataEntry.serviceInterfaces map onto the host → binding tree is a nice simplification. The commentary on the non-obvious invariants (the secure: true gate, exact-source exclusion in overlap, WAN-opt-in default) is genuinely helpful.

Verified

  • SDK tests: 70/70 pass (make test-sdk), including the new RangeOrigin.export / createRangeInterface coverage.
  • Rust tunnel tests: 14/14 pass, including port_forward_overlap_detection. The interval-overlap logic (new_lo <= hi && lo <= new_hi, same-IP filter, exact-source exclusion, saturating arithmetic) is correct and the test matrix is thorough (straddle, swallow-SNI, adjacent-disjoint, other-WAN, exact-key).
  • ts-bindings: regenerated — no drift vs. committed.
  • CI: all Compile Base Binaries, Build Debian Package (every arch incl. apple-darwin + musl), Run Automated Tests, and Formatting jobs are green (ISO Build Image jobs still assembling at time of writing, 0 failures).
  • Static checks: WAN opt-in default confirmed — DerivedAddressInfo::enabled() excludes public raw IPs unless explicitly enabled, so a freshly-bound range is LAN-only until the operator toggles WAN on, preserving the old RangeGatewayAccess::Lan default. i18n is balanced across all 5 dictionaries.

One concrete fix

  • projects/start-sdk/docs/src/interfaces.md:245 still describes the model this PR removed: "Range interfaces show up in the service's Interfaces page with a per-gateway LAN / LAN+WAN access control; choosing LAN+WAN prompts the operator…". That's the old tri-state UX — per the PR they now render as per-address toggle cards identical to single-port. Worth rewording to match. Happy to push a one-liner to the branch if you want.

Confirm (likely intended)

  • find_service_interface / list_all_service_interfaces scan bindings only, so range interfaces are excluded from getServiceInterface/listServiceInterfaces effects (they're read off the host model instead). Reads as intentional given a range has no AddressInfo — just flagging in case any consumer expects to enumerate them via the effect.

Couldn't test end-to-end (no VM coverage)

I can't exercise the headline runtime path in a VM here: it needs a full StartOS image built from this branch (the StartOS web UI changed too, so a binary-only deploy would mismatch) and a package that calls the new bindPortRange/createRangeInterface surface — no such package exists yet, and that surface is on the unreleased local SDK. So these still want a hardware/QA pass:

  1. A fresh range shows in the service Interfaces page as LAN-only address cards; enabling the WAN address is what exposes it (and only then emits the router port-forward hint).
  2. Toggling a range's WAN address on/off actually reconciles — the explicit update_addresses recompute that the old set_range_gateway_access did was dropped, now relying on the same Hosts-DB-watcher path single-port uses. Worth confirming the derived port_forwards hint and the nft DNAT both update for ranges, not just single-port.
  3. The nft DNAT maps the full external→internal span correctly when externalStartPort != internalStartPort (offset mapping), and the manual port-forward add --count path installs/persists as expected (PCP PORT_SET).

If you'd like, I can take a crack at the full image build + a minimal port-range test package to close out 1–3 as a follow-up — just say the word.

Comment thread shared-libs/crates/start-core/src/net/host/binding.rs Outdated
…points

Per @dr-bonez: single-port bindings and port ranges live in separate DB
subtrees, so treat them differently at the API instead of probing both maps.
set-address-enabled now touches only the single-port `bindings`; a sibling
set-range-address-enabled touches `bindingRanges`. Both share the param struct
and the set_address_enabled_on cascade. The UI picks the endpoint by the
address's port span; mock split to match.
@helix-nine helix-nine force-pushed the feat/port-range-interfaces branch from 1304ab0 to a1405bc Compare June 30, 2026 21:56
helix-nine added a commit that referenced this pull request Jun 30, 2026
Reconciles the rebase onto the two-endpoint base (#3366): set_range_address_enabled
shares BindingSetAddressEnabledParams, whose address became CliFromJsonString in
this PR, so it reads address.0 like set_address_enabled rather than re-parsing a
String.

@helix-nine helix-nine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adversarial review — PR #3366

Ran this through a multi-agent adversarial review (9 review dimensions fanned across the diff; every finding was then refuted by 3 independent skeptics with distinct lenses — correctness / is-it-actually-in-this-diff / real-impact — and kept only if ≥2 of 3 judged it real). 27 candidate findings raised → 16 survived. I then personally re-verified the Critical against the actual feat/port-range-interfaces code (confirmed below).

C1 spot-check (confirmed): net_controller.rs range loop builds public_gateways from enabled_addresses.filter(|a| a.public) — no && is_ip() — and forces ForwardRequirements { secure: true }, which bypasses forward.rs's !reqs.secure && !info.secure() non-secure-gateway drop. DerivedAddressInfo::enabled() default-enables a PublicDomain (it's public but not an IP, so it takes the !disabled.contains(...) branch). The single-port non-SSL path is safe only because it passes secure: false. So on a server with any clearnet domain, a freshly-bound range is DNAT'd to the WAN (no source filter) on first launch.


Review: PR #3366 — Port-range interfaces + StartTunnel range forwarding + set-address-enabled refactor

Overall assessment

The mechanical refactor (two-endpoint split, repo-reference cleanup, the tunnel overlapping() guard with its unit test) is solid, but the address-model rewrite for port ranges has a serious security regression: the new per-address enable/disable model does not extend its WAN-opt-in default to public domains, so a freshly-bound range becomes raw-DNAT'd to the WAN the moment the server has any clearnet domain — the exact opposite of the feature's stated "WAN is opt-in" invariant, and a posture the old RangeGatewayAccess-default-Lan design did not have. The riskiest area by far is net/host addressing + net_controller forwarding; the docs for the headline feature are also broken (the canonical example doesn't type-check). Tunnel forwarding and UI are mostly correct with a few labeling/visibility gaps.


Critical

C1. Fresh port range is WAN-exposed by default whenever a public domain exists

One-liner: The WAN-opt-in default-off rule covers only public IPs, not public domains, so a range auto-opens its entire port span to the internet with zero operator action.
Where: shared-libs/crates/start-core/src/net/host/mod.rs Model<Host>::update_addresses range loop (PublicDomain synthesis, ~L416–426 / ~L516–530) + net/host/binding.rs DerivedAddressInfo::enabled() (L127–147) + net/net_controller.rs range forward loop (~L513–566, secure: true).
Failure scenario: enabled()'s disabled-by-default predicate is h.public && h.metadata.is_ip(). A synthesized range address with public: true, metadata: PublicDomain is public but not an IP, so it falls into the else branch (!disabled.contains(...)) and is enabled by default on a fresh range (empty disabled set). net_controller then puts its gateway into public_gateways and forwards the range with ForwardRequirements { secure: true }. In forward.rs, secure: true bypasses the !reqs.secure && !info.secure() non-secure-gateway drop, and public_gateways.contains(gw_id) sets src_filter = None → the whole span is DNAT'd to the WAN with no source filter. Any service calling bindPortRange (coturn RTP, FTP data) auto-exposes on first launch on any server that has a clearnet domain — the normal way operators expose services. The equivalent non-SSL single-port binding is safe because it uses secure: bind.options.secure.is_some() (false), so its public domain is dropped by the same gate; ranges force secure: true and lose that backstop. The in-code comment "A fresh range's public (WAN) addresses are disabled by default" is therefore false.
Fix: Make the WAN-opt-in default cover all public addresses, not just IPs — change the default-off predicate in enabled() to h.public so a PublicDomain must be explicitly present in the enabled set before it contributes. Alternatively, in the net_controller range loop only add a public address to public_gateways when its WAN SocketAddr is explicitly in addresses.enabled (since secure: true removed the other backstop). Re-verify the "disabled by default" comment actually holds for domains afterward. (This is the merged form of the security-dimension critical and the backend-addressing-dimension high describing the same defect.)


High

H1. Non-exported ranges are forwarded but invisible in the UI — no way to observe or disable their exposure

One-liner: The interfaces UI only renders ranges that exported an interface, but the backend forwards every enabled range regardless.
Where: projects/start-os/web/ui/.../interfaces.component.ts ranges = computed(...) (~L162): Object.entries(host.bindingRanges).filter(([, range]) => range.interface); backend net_controller.rs iterates all host.binding_ranges independent of range.interface.
Failure scenario: A package binds a raw range via bindPortRange without calling RangeOrigin.export (a normal pattern for RTP/data ranges). The firewall forwards it, but it has no card anywhere in the UI — the dedicated port-ranges route/components were deleted in this PR. Combined with C1, such a range can be silently WAN-exposed with no UI surface to see it or to toggle the public address off via set-range-address-enabled. Even without C1, the operator cannot audit or LAN-restrict a non-exported range from the UI; a control reachable only via the API is effectively absent for most operators.
Fix: Render all entries of host.bindingRanges (port span + per-address enabled/disabled toggles), not only those with range.interface. The interface field should drive labeling/scheme only, not visibility.

H2. "Port Ranges" docs example returns void[] from setupInterfaces and won't type-check

One-liner: The canonical onboarding example for the headline feature does not compile.
Where: projects/start-sdk/docs/src/interfaces.md, Port Ranges section, first code block (~L205–224): return [ await range.export(...) ].
Failure scenario: RangeOrigin.export() returns Promise<void> (shared-libs/ts-modules/start-core/lib/interfaces/RangeOrigin.ts), unlike single-port Origin.export() which returns Promise<AddressInfo[] & AddressReceipt>. sdk.setupInterfaces constrains the callback return to Array<T.AddressInfo[] & AddressReceipt> (setupInterfaces.ts). [ await range.export(...) ] is void[], which isn't assignable → tsc error for any author copying the example. It also contradicts the second example in the same section (and host.test.ts), which correctly awaits the export as a bare statement and does not collect it.
Fix: Await the range export as a statement and return only single-port receipts (or []):

await range.export(sdk.createRangeInterface(effects, { id: 'turn-relay', name: i18n('TURN Relay'), description: i18n('WebRTC media relay ports') }))
return []

Make both examples in the section consistent. (Two skeptics raised this independently from sdk and cross-cutting; same defect.)


Medium

M1. interfaces.md still documents the removed LAN / LAN+WAN access-control model

One-liner: The Port Ranges section describes a per-gateway LAN/LAN+WAN selector that this very PR deletes.
Where: projects/start-sdk/docs/src/interfaces.md, Port Ranges section, L245.
Failure scenario: The doc says ranges show "a per-gateway LAN / LAN+WAN access control; choosing LAN+WAN prompts…". But this PR removes RangeGatewayAccess (disabled/lan/lan-wan), the gatewayAccess map, the set-range-gateway-access subcommand, and the RangeGatewayAccess.ts/BindingSetRangeGatewayAccessParams.ts bindings, replacing them with RangeBindInfo.addresses: DerivedAddressInfo + set-range-address-enabled and a per-address enable toggle in the UI. Authors/operators are told about a dropdown that doesn't exist.
Fix: Rewrite to the shipped model, e.g. "Range interfaces appear on the Interfaces page using the same per-gateway address card as single-port interfaces (non-SSL, IPv4-only). The public/WAN address is disabled by default; enabling it surfaces the exact port range to forward on the router."

M2. Port-range "Address Requirements" modals show the external range in the "Internal Range" column

One-liner: Both External Range and Internal Range cells render the same external-start value, so the operator gets a wrong internal range.
Where: projects/start-os/web/ui/.../interfaces/addresses/gateway/port-forward.component.ts (template ~L50–51 / ~77–83, portDisplay ~L217) and the identical pattern in dns.component.ts (~L50–51 / ~77–83, portDisplay ~L350).
Failure scenario: portDisplay = formatPortRange(context.data.port, count) where context.data.port is the external start port (from address().hostnameInfo.port = externalStartPort). The internal start port is never plumbed into the modal (openPortForwardModal/openPublicDomainModal receive only { gateway, port, count, initialResults }). For external 49152–49251 → internal 5000–5099 (SDK Host.ts: externalStartPort + iinternalStartPort + i), the dialog reports Internal Range = 49152–49251. The operator configures router/firewall forwarding against a bogus internal range. Single-port modals were correct because single-port forwards keep src.port == dst.port (mod.rs:473–474); ranges break that. Mock data hides it (externalStartPort == internalStartPort).
Fix: Plumb the internal start port (available as internalStartPort = Number(key) in interfaces.component.ts, and as addressInfo.internalPort) through DomainHealthService.showPortForwardSetup/showPublicDomainSetupopenPortForwardModal/openPublicDomainModal into the modal data, then compute the range branch's Internal Range as formatPortRange(internalPort, count); keep External Range as formatPortRange(externalPort, count).

M3. set_range_address_enabled no longer recomputes host.port_forwards — the router-rule list goes stale

One-liner: Enabling a range's WAN address installs the DNAT but leaves the operator-facing "add this rule on your router" list out of date.
Where: shared-libs/crates/start-core/src/net/host/binding.rs set_range_address_enabled (~L378–426, replacing set_range_gateway_access).
Failure scenario: The removed set_range_gateway_access explicitly recomputed derived port_forwards (Kind::host_for(...).update_addresses(&hostname, &gateways, &ports), with a comment about reflecting the change immediately). The replacement only calls set_address_enabled_on(&mut range.addresses, ...) and drops the recompute. The net_controller Hosts watcher runs data.update() (reconciles iptables from live enabled()) but not update_addresses, so port_forwards (a computed DB field consumed by port-forwards.component.ts) stays stale until an unrelated bind/network event re-runs update_addresses. The operator sees no port-forward instruction for a range they just exposed → inbound WAN traffic silently fails.
Fix: Restore the update_addresses recompute inside set_range_address_enabled after mutating range.addresses. Check the single-port set_address_enabled for the same gap — if it's known-good via another trigger, note why; otherwise fix both.

M4. SNI-add path lacks the range-overlap check — an SNI forward can be placed inside an existing DNAT range

One-liner: The new overlapping() guard runs only on the DNAT-add path; the SNI branch returns before it.
Where: shared-libs/crates/start-core/src/tunnel/api.rs add_forward SNI branch (early return ~L1036, before the overlapping() check); mirrored in forward/pcp.rs add_sni_forward.
Failure scenario: add_forward's SNI branch returns before the overlap check, and add_sni_forward only rejects an exact-source DNAT collision (pf.0.entry(source) => Dnat => error). Adding an SNI at 1.2.3.4:8005 while a DNAT range 1.2.3.4:8000 count=10 already covers 8005 is accepted: a new Sni entry is stored at :8005 and the SNI demux registers there, while the range's nft DNAT already redirects that port — the two forwards disagree on who owns 8005. The reverse (DNAT range swallowing an existing SNI) is caught because PortForward::Sni.port_span() == 1 and overlapping() runs on DNAT-add (unit-tested). The check is one-directional.
Fix: Run overlapping(source, 1) before persisting an SNI forward, skipping an existing Sni at the exact source (the legitimate add-route case). In add_forward move/duplicate the overlap check to also guard the SNI branch; in add_sni_forward extend the DB mutate to reject a different-start DNAT range covering source.port().

M5. getServiceInterface now watches the package's entire hosts subtree — callback over-fires on unrelated recomputation

One-liner: Widening the watch from one interface to the whole hosts tree wakes every dependent on every network blip.
Where: shared-libs/crates/start-core/src/service/effects/net/interface.rs get_service_interface (ptr /public/packageData/{pkg}/hosts, L179–198).
Failure scenario: Previously the callback watched /public/packageData/{pkg}/serviceInterfaces/{id} (fires only on that interface). It now watches /public/packageData/{pkg}/hosts and resolves via find_service_interface, so the DbWatchedCallbacks::add callback fires on any change under hosts — which update_addresses rewrites on every gateway/IP/DHCP/mDNS/domain change (bindings/*/addresses/available, port_forwards). Each wakeup costs an extra getServiceInterface + getHostInfo RPC round-trip per dependent, scaling with dependents × network-event rate. The JS Watchable deepEqual debounce prevents incorrect UI churn, so this is a CPU/RPC overhead regression, not a correctness bug. (list_service_interfaces already watched the whole map, so its scope is roughly unchanged — this is specific to the single-get path.)
Fix: Narrow the watch to the interface's binding location (/public/packageData/{pkg}/hosts/{hostId}/bindings/{port}/interfaces/{id}) when resolvable, falling back to the broad watch only when not yet found; or compare the resolved ServiceInterface before/after in the callback and suppress when unchanged.

M6. On range resize/move, preserved disabled overrides go stale (keyed by old external port), silently re-enabling disabled public domains

One-liner: A service-driven re-bind that changes the port span resurrects a WAN address the operator had disabled.
Where: shared-libs/crates/start-core/src/net/host/mod.rs add_binding_range (~L638–665): let addresses = existing.map(|e| e.addresses.clone()).unwrap_or_default(); carried across a !unchanged rebind that changes external_start_port.
Failure scenario: An operator disables a range's public-domain address, recording (domain, old_external_start_port) in addresses.disabled. On a non-idempotent rebind (package bumps number_of_ports or external_start_port), the whole addresses struct (including disabled) is preserved verbatim, but update_addresses recomputes available against the new start port. The stale disabled key matches nothing, so enabled() re-includes the public domain → the range is WAN-exposed again after the resize, with no notification. (Compounds C1.)
Fix: When !unchanged and the span moves, re-key the preserved enabled/disabled overrides to the new external_start_port — or, simpler, key range overrides by (hostname)/gateway rather than (hostname, port), since all range addresses share one representative port. At minimum, drop overrides that can no longer match and re-apply the safe default (LAN-only after C1's fix) rather than letting a stale-disabled domain flip back to enabled.


Low / Nit

L1. DNS "Server" column shows non-server values for CNAME/TXT/custom records

Where: projects/start-tunnel/web/src/app/routes/home/routes/dns/index.ts serverDisplay() (~L197–207) + Manual table header/cell (~L45, ~L55–58).
Scenario: The column was relabeled "Value" → "Server" and every row renders via serverDisplay(record), which looks up record.value in devices() and falls back to returning record.value verbatim. The add dialog (dns/add.ts, TYPES = ['A','AAAA','CNAME','TXT']) creates CNAME (hostname value), TXT (arbitrary text), and "Other (custom)" A/AAAA (arbitrary IP). For these, record.value is not a server IP, so a TXT string / CNAME target / custom IP appears under a "Server" header.
Fix: Keep the header "Value" (generic and correct for all types), or branch in serverDisplay to resolve server name/IP only for A/AAAA values matching a known server and render record.value plainly otherwise.

L2. Existing forward rows targeting clients now render with raw IP after the devices picker was narrowed to servers

Where: projects/start-tunnel/web/src/app/routes/home/routes/port-forwards/index.ts devices signal (~L212–224), consumed by mapForwardsutils.ts toRow() (lookup at L77).
Scenario: The devices signal is now filtered to c.kind === 'server'. It feeds both the Add dialog's Server picker (correct) and mapForwards(this.portForwards(), this.devices()), which resolves each row's name via devices.find(d => d.ip === targetip). A forward whose target is now a client (the diff even adds a "Change to Client" action) no longer resolves → utils.ts falls back to { ip, name: ip }, showing the bare IP instead of the friendly name. Degrades gracefully (no crash) but is a visible label regression.
Fix: Keep a full-device-list signal for name resolution in mapForwards, and pass only the server-filtered list to the Add dialog's picker (context.data.devices). Don't conflate "selectable as a new target" with "name lookup for an existing target."

L3. RangeOrigin.export silently overwrites a prior range interface — "exactly one interface" documented but not enforced

Where: shared-libs/ts-modules/start-core/lib/interfaces/RangeOrigin.ts export(); Rust handler export_range_service_interface in service/effects/net/interface.rs.
Scenario: Docs/CHANGELOG state a range exposes "exactly one" interface, but export can be called repeatedly; the Rust handler does .as_interface_mut().ser(&Some(interface)) (unconditional overwrite of binding_ranges[start].interface). A second export silently replaces the first with no error, and both ids get pushed into the interfaces tracking array though only the last is stored — a mismatch with what clearServiceInterfaces is told to keep.
Fix: Throw on a second export of the same RangeOrigin, or have the Rust handler error when interface is already Some with a different id in the same setup pass. At minimum, document that re-export overwrites.

L4. Tunnel range-forward overlap check is read-then-write (TOCTOU)

Where: shared-libs/crates/start-core/src/tunnel/api.rs add_forward (~L1010–1060): db.peek()...overlapping(source, count) then a separate add_forward_range(...) + active_forwards.mutate(...); same in tunnel/forward/igd.rs apply_peer_forward_range.
Scenario: Two concurrent add_forward calls (or an auto peer-forward racing a manual add) can both peek the pre-insert state, both see no overlap, and both install forwards whose external spans overlap on the same WAN IP — defeating the new check. The overlapping() logic itself is correct; the race is the detached peek/mutate boundary. Low severity: tunnel RPCs are low-concurrency and the worst case is a duplicated forward, not a boundary breach.
Fix: Move the overlap check inside the same db.mutate that performs the insert, so check and insert serialize against concurrent writers.

L5. Orphaned i18n key 882 "Port Range" added to all 5 locales but referenced nowhere

Where: shared-libs/ts-modules/shared/src/i18n/dictionaries/en.ts key 'Port Range': 882 (+ de/es/fr/pl 882).
Scenario: Key 882 was added to all five dictionaries, but no UI source under projects/start-os/web/ui/src references the string (no | i18n usage). Sibling keys 883 (External Range), 884 (Internal Range), 885 (PCP help) are used; only 882 is dead. start-tunnel uses raw English, so it isn't the consumer.
Fix: Either wire "Port Range" into the component that needs it (range card / column header), or drop key 882 from all five dictionaries.


Coverage / caveats

Reviewed (static): the net/host addressing model (mod.rs update_addresses/add_binding_range, binding.rs enabled()/set_range_address_enabled), net_controller.rs range forwarding and forward.rs gating, the tunnel overlapping() guard and its DNAT/SNI/PCP add paths, the getServiceInterface watch-scope change, the SDK RangeOrigin/setupInterfaces types and the packaging docs, and the start-os + start-tunnel UI components for ranges, port-forwards, and DNS. The two-endpoint split, repo-reference cleanup, and the overlapping() unit test all look correct.

Warrants human/QA or runtime/VM verification (static review can't settle):

  • C1 / H1 firewall behavior — highest priority. Confirm on a VM that has a clearnet public domain: bind a range (with and without an exported interface), then inspect the actual nft ruleset (nft list ruleset) to verify whether the span is DNAT'd to the WAN with no source filter on first launch, and whether the public address shows as enabled-by-default. This is the make-or-break check for the critical finding.
  • M6 resize behavior. Disable a range's WAN address, then trigger a non-idempotent rebind (widen number_of_ports) and confirm whether the address flips back to enabled.
  • Upgrade/migration on real data. Two related items were refuted in adversarial verification (no migration for removed RangeGatewayAccess; serviceInterfaces field removed from PackageDataEntry). They didn't survive 2/3 skeptics, but the common theme — this PR removes persisted DB fields/enums without a migration — is worth a deliberate upgrade test on a node with existing range opt-ins and exported interfaces, to confirm nothing is silently dropped or mis-read on first boot of the new build.
  • M3 stale port_forwards. Verify in the running UI that enabling a range's WAN address immediately updates the "add this rule on your router" list, vs. staying stale until an unrelated event.

Honest bottom line: there is exactly one Critical (C1) and two High (H1, H2). C1 is a genuine default-on WAN exposure and should block merge until the enabled() predicate is fixed and verified on a VM; H1 compounds it by hiding the exposure from the UI. The rest are real but non-blocking. No findings were padded — the refuted set was left out except for the migration caveat above.

@helix-nine

Copy link
Copy Markdown
Contributor

Correction / downgrade on C1, per @dr-bonez: a public domain isn't auto-discovered like an IP — the operator adds it, and that add is the intent to expose. So default-enabling an added domain (and forwarding the range) is correct, not a default-on bug. My "zero operator action" framing was wrong, and "the server has any clearnet domain" overstated the scope — public_domains is per-host (operator-added per host via add_public_domain), not server-wide. Withdrawing C1 as a Critical.

One narrow residual worth a confirm (not blocking): public_domains is stored host-level, but add_public_domain is invoked for a target internal_port. So a domain added for binding A on host X is synthesized + default-enabled (enabled()'s non-IP-public branch) onto a port range also on host X — and because the range forward loop forces ForwardRequirements { secure: true } (vs single-port non-SSL's secure: bind.options.secure.is_some() = false), only the range actually gets WAN-DNAT'd from that spillover; a co-located single-port non-SSL binding stays gated by !reqs.secure && !info.secure(). Net: adding a domain to expose, say, an SSL UI on a shared host would also expose a raw non-SSL range on it.

If co-locating a range with other bindings on a single host isn't a supported/expected pattern (ranges get their own MultiHost), this is moot — fully withdrawn. Otherwise it's a "host-level domain exposes co-located ranges (only ranges, via the secure:true asymmetry)" edge to confirm intended. H1 (non-exported ranges have no UI card to view/LAN-restrict) stands on its own regardless.

…on boot

Per @dr-bonez, addressing the review's H1 / C1-residual: a binding or range
with no exported service interface should listen internally only, not be
forwarded.

- net_controller forward/vhost loops and the host/mod.rs port-forward-hint loops
  skip any single-port binding with empty `interfaces` and any range with
  `interface: None`. The container still binds the port on lxcbr0; StartOS just
  doesn't expose it on any gateway.
- The OS's own server binding has no package to run setupInterfaces, so
  os_bindings() now syncs the `startos-ui` ServiceInterface into it on every
  boot (idempotent — the OS's equivalent of setupInterfaces). This covers fresh
  and upgraded servers with no migration, so the admin UI is never gated out.
helix-nine added a commit that referenced this pull request Jun 30, 2026
Reconciles the rebase onto the two-endpoint base (#3366): set_range_address_enabled
shares BindingSetAddressEnabledParams, whose address became CliFromJsonString in
this PR, so it reads address.0 like set_address_enabled rather than re-parsing a
String.
Comment thread shared-libs/crates/start-core/src/net/host/mod.rs Outdated
Comment thread shared-libs/crates/start-core/src/net/net_controller.rs Outdated
Comment thread shared-libs/crates/start-core/src/net/net_controller.rs Outdated
…bindings stay internal-only

Loopback and the lxcbr0 bridge (HOST_IP) appear in every binding's
addresses.available and are how the host and other containers reach a
service. They must never be operator-disablable, and a binding/range with
no exported interface must still be reachable on them.

- HostnameInfo::is_internal(): loopback or HOST_IP, IP-parsed from hostname
- DerivedAddressInfo::enabled(): always includes internal addresses
- set_address_enabled / set_range_address_enabled: reject disabling internal
- net_controller: a non-exported binding/range no longer skips forwarding
  entirely; it forwards only its internal addresses (lo/lxcbr0), never a
  gateway
@helix-nine

Copy link
Copy Markdown
Contributor

Pushed 5955561d2 addressing the lo/lxcbr0 handling (and rebased #3368#3373 on top).

Non-exported bindings are now internal-only, not dropped. Both net_controller gates changed from continue to a filter: when interfaces.is_empty() (single-port) or interface.is_none() (range), enabled_addresses is restricted to is_internal() — loopback + lxcbr0/HOST_IP — so the binding still gets a vhost + local forward on lo/lxcbr0, just never a gateway.

lo/lxcbr0 can no longer be disabled. New HostnameInfo::is_internal() (IP-parsed: v4 loopback-or-HOST_IP, v6 loopback). DerivedAddressInfo::enabled() always includes internal addresses (a first branch, ahead of the GUA/public/disabled logic), and both set_address_enabled / set_range_address_enabled reject enabled=false for an internal address with InvalidRequest.

No UI change was needed. Internal addresses never render a toggle: gatewayService.gateways() already filters out deviceType bridge/loopback, so a loopback/bridge address's gateway id isn't in the interfaces gateway list and the row is skipped before any switch is drawn. The backend RPC + enabled() guard fully enforce the invariant (incl. against the CLI/raw API). I drafted a UI internal flag but reverted it as dead code — happy to add a visible-but-locked row instead if you'd rather surface them.

cargo check clean, net::host 27 passed, check:ui green.

…mmands

The device set-wan/set-kind/set-auto-port-forward and subnet set-wan
subcommands (added in the monorepo reorg) had no committed man pages.
Regenerated via the export_manpage_start_tunnel test.
@helix-nine

Copy link
Copy Markdown
Contributor

Also added the 4 missing start-tunnel man pages (device set-wan/set-kind/set-auto-port-forward, subnet set-wan) — those subcommands landed in the monorepo reorg without regenerated pages. Regenerated via the export_manpage_start_tunnel test; diff is purely the 4 new files (no existing pages changed, so the output is deterministic). Committed on this PR (77f678775) and rebased the #3368#3373 stack on top.

A binding/range with no exported service interface must not hold an
enabled public (WAN) address. Enforce it in the data model rather than
skipping such bindings in the port-forward computation:

- set_address_enabled / set_range_address_enabled reject enabling a
  public address when the binding/range has no exported interface.
- update_addresses clears the (public-only) enabled set for any
  non-exported binding/range, self-healing a binding that lost its
  interface after the operator had opted a WAN address in.
- The port-forward hint loops no longer special-case non-exported
  bindings — with no enabled public address they contribute nothing.

Runtime forwarding of a non-exported binding to lo/lxcbr0 is unchanged
(net_controller filters its enabled addresses to internal-only).
Mirror the old sdk.serviceInterface.get/getOwn ergonomics: GetHostInfo is
now generic over the mapped type and takes { map, eq } options, and new
getOwnHost/getHost helper overloads expose an optional map (and eq,
default deep-equal) selector. With const(), the calling context re-runs
only when the mapped child attr changes instead of on any change to the
whole host.

sdk.host.getOwn/get delegate to the helpers (direct-assigned, preserving
the overloads). Reactivity narrowing is inherited unchanged from
Watchable — the same path sdk.serviceInterface used.
@helix-nine

Copy link
Copy Markdown
Contributor

Added map/eq selective reactivity to sdk.host.get/getOwn (8d992cf59), restoring the ergonomics the old sdk.serviceInterface.get/getOwn had:

  • GetHostInfo is now generic over the mapped type and takes { map, eq } options; new getOwnHost/getHost overloads expose an optional map (and eq, default deep-equal) selector — a direct mirror of getOwnServiceInterface/getServiceInterface.
  • sdk.host.getOwn/get direct-assign the helpers, preserving the overloads.
  • The reactivity narrowing is inherited unchanged from Watchable (the same path serviceInterface used), so with const() the context re-runs only when the mapped child attr changes:
    const ui = await sdk.host
      .getOwn(effects, 'ui', h => h?.bindings[80]?.interfaces['ui'])
      .const()

Docs (main.md) + the 2.0.0 CHANGELOG entry updated. tsc clean on the ts-module + SDK, check:ui green. Rebased #3368#3373 on top.

…ot mutation

The enabled/disabled sets are operator overrides of the per-address
default; a reconciler must not rewrite them. Replace the update_addresses
prune (which cleared the public-only enabled set — destructive on
interface removal, and a no-op for the LAN/mDNS/domain defaults) with a
read-side derivation: BindInfo/RangeBindInfo::enabled_addresses() returns
enabled() filtered to internal (lo/lxcbr0) when no interface is exported.
net_controller and the port-forward hint both use it, so a non-exported
binding serves internally only while its overrides stay stored and
dormant, taking effect again when an interface returns.

Also drops the set_address_enabled / set_range_address_enabled
public-address rejects (a dormant override is harmless); keeps the
can't-disable-lo/lxcbr0 guard.
@helix-nine

Copy link
Copy Markdown
Contributor

Reworked the non-exported-binding handling per @dr-bonez — the earlier approach mutated an override set instead of changing the default (e80d6fe01).

The enabled/disabled sets are operator overrides of the per-address default, so the reconciler must not rewrite them. The old update_addresses prune (enabled.clear() when no interface) was both destructive — it wiped the operator's WAN opt-in on interface removal — and a no-op for the LAN/mDNS/domain defaults (they aren't in enabled).

Replaced it with a read-side derivation: BindInfo/RangeBindInfo::enabled_addresses() returns enabled() filtered to internal (lo/lxcbr0) when no interface is exported. Both net_controller and the port-forward hint use it, so a non-exported binding serves internally only while its overrides stay stored and dormant — they light back up when an interface returns. No data mutation.

Also dropped the set_address_enabled / set_range_address_enabled / set_gua_access WAN-enable rejects (a dormant override is harmless), keeping only the can't-disable-lo/lxcbr0 guard. Rebased #3368 (dropped its now-moot LAN+WAN-GUA reject) and #3373. net:: tests green across the stack.

@helix-nine

Copy link
Copy Markdown
Contributor

Thanks for the review. #3366 is the base of the stack (→ #3368#3373-draft), all three linear and net:: green. Whenever you merge this one down, ping me and I'll rebase #3368 (and #3373) onto master — or I can do it now if you'd rather they track master ahead of the merge.

@dr-bonez dr-bonez merged commit d0ee03e into master Jul 1, 2026
16 checks passed
@dr-bonez dr-bonez deleted the feat/port-range-interfaces branch July 1, 2026 16:50
@helix-nine

Copy link
Copy Markdown
Contributor

Merged — thanks. Followed through on the rebase: #3368 is now on master (dropped the squashed #3366 commits, resolved the CHANGELOG collision so Unreleased carries just the GUA tri-state entry, not the already-released monorepo/backup-fs ones), and #3373 is rebased on top of it. Both linear and synced, net:: green (27 / 151). #3368 (df10f6a79) → #3373 (afed01303, still draft).

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.

3 participants