Repository navigation
fix(portal): drop nodogsplash gatewaydomainname (5.0.2 splash redirect loop) + brand-selected hostname - #432
Conversation
…-aware portal URL in both revert messages The #432 rework removed the originalDomain variable but left its final Printf referencing it — the CLI module did not compile (go vet: ssl.go: undefined: originalDomain). The farewell message now derives the portal URL from the system hostname like the rest of the brand-selection work; the self-signed revert's hardcoded TollGate.lan gets the same treatment so net4sats builds no longer print the wrong name.
|
Second-agent verification pass (full diff read):
One validation note for the maintainer: this is pre-auth-surface behavior on real NDS 5.0.2 — the QEMU-router lane (the same one that validated #412) or a physical spot-check is the right confirmation before stable; the cloud-lab's fake-ndsctl can't exercise the redirect path. From the diff's logic: verified. |
felixfelix-bot
left a comment
There was a problem hiding this comment.
Reviewed head: f1e947512d — merge-base 125f6a06; no conflicts (4 ahead / 0 behind main; mergeable_state: blocked is the pending-review gate: 1 requested reviewer, 0 reviews).
What it does. Removes nodogsplash.@nodogsplash[0].gatewaydomainname from first-boot setup and from tollgate ssl (revert now deletes it instead of restoring a hardcoded TollGate.lan), and makes /etc/tollgate/brand the single source for hostname, SSID prefix and NDS gateway name.
The mechanism is real on the venue, not on stock firmware. NDS only sees that option if the device's init script is patched. Upstream's conf generator copies an explicit whitelist (net/nodogsplash/files/etc/init.d/nodogsplash, generate_uci_config(): daemon debuglevel maxclients gatewayname …) containing no gatewaydomainname — 0 hits in all three revisions of that file — and that conf is the only input procd gives the daemon (-c /tmp/etc/nodogsplash_$cfg.conf, line 190). Your repo says the same at 90-tollgate-captive-portal-symlink:56-59. The venue patches the device: physical-router-test-automation/lib/router.py:159-167 seds gatewaydomainname into /etc/init.d/nodogsplash and restarts it, called from lib/clients/wifi.py:369-370 whenever the option is set. So #428's loop is harness-induced: firmware writes the option, the hook patches the init, NDS honours it. "Every current install is affected" overstates device impact; please add stock-device evidence (grep -c gatewaydomainname /etc/init.d/nodogsplash, /tmp/etc/nodogsplash_*.conf) — if zero, the setting was inert and any real-device loop needs another root cause. The harness assertion test_setup_script.py::test_setup_nodogsplash_gatewaydomainname then silently skips.
The delete never runs when the version string is unchanged. It sits in setup_nodogsplash (:184), but the script exits at :388 when EXISTING_VERSION equals SETUP_VERSION (:369-388), before the call at :401. SETUP_VERSION comes from packaging/Makefile:166, and VERSION has been v0.6.0-alpha2 since #380 — so installing this build over any post-#380 device leaves the option in place, though packaging/postinst:18-24 and /etc/init.d/boot:7-20 both re-run the script. "Actively deleted on upgrade" holds only for version-changing upgrades.
The brand selector does not move the hostname on any non-fresh device. setup_hostname assigns only when the hostname is exactly OpenWrt (:137-146), while SSID (:128) and NDS gateway name follow the brand on every version-changing setup: an existing device gets Net4sats-XXXX SSIDs with a TollGate hostname and uhttpd.main.commonname (:147-148), so net4sats.lan never resolves. Both venue happy paths were fresh installs, so the lane cannot catch it. Minor: a typo'd brand value is silently coerced to TollGate, unlogged.
Acquittals: the revert cannot abort — CombinedOutput() in runCommandChecked (ssl.go:30-43) puts uci's Entry not found in the error string, so the guard at :833-843 returns nil and nothing is left uncommitted; a hostname change cannot invalidate an issued cert; <hostname>.lan does resolve via dnsmasq (add_local_hostname on by default; domain='lan'/expandhosts='1' at :97-103); sh -n, busybox ash -n, shellcheck -s sh and go build ./... are clean — syntax only, which cannot see the branch gap above. Unverified by me: "go vet green" (my run: "main module does not contain package …/cmd/tollgate-cli"), and the venue's NDS package (no nodogsplash on openwrt/packages openwrt-24.10/openwrt-25.12 — 404).
CI: no GitHub CI evidence — gh pr checks 432: "no checks reported on the 'fix/428-nds-gatewaydomainname-loop' branch"; runs API total_count: 0 for head_sha=f1e9475…; nothing has completed since 2026-08-27T16:45:13Z (newest run, 2026-09-17, is status: queued/conclusion: null). The ngit lane (.ngit/act) runs on main only; no result for this branch.
RC RECOMMENDATION: LAND BEFORE THE ALPHA RC. It rewrites 99-tollgate-setup and the shipped tollgate-cli, so it changes the artifact: after the freeze it either waits a cycle or forces a re-cut plus a full retest of the frozen release commit. Landed before, the RC is retested once and ships the phone happy path that #428 blocked working in the lane that gates the RC. The two gaps are follow-ups, not blockers, but the freeze should not carry the CHANGELOG's "actively deleted on upgrade" and "every current install is affected".
|
Follow-up: the QEMU-lane confirmation I asked for above is done — fix validated live on real nodogsplash 5.0.2. Lab: PRTA virtual lab (OpenWrt 24.10.1 x86-64 VM, Bug reproduced (pre-fix): fetching the splash via the friendly domain from an unauthenticated client — (Plain-IP splash fetches serve 200 — the loop is specifically the gatewaydomainname rewrite path, matching the PR's mechanism.) Fix validated:
From our side this PR is now logic-verified and behavior-confirmed on the affected NDS version. |
Amperstrand
left a comment
There was a problem hiding this comment.
Ran src/cmd/tollgate-cli on this branch: go vet + go test -race -count=1 -tags testenv ./... green. Read the diff: the uci-defaults change deletes gatewaydomainname (the NDS 5.0.2 redirect loop trigger per #428) and keeps .lan resolution via dnsmasq's hostname serving; the brand file is a read of /etc/tollgate/brand with a sane default; the RANDOM_SUFFIX ${GATEWAY_NAME#*-} generalization is correct for brand-prefixed SSIDs; ssl.go stops setting the option and cleans it on revert. Low-risk for older NDS (deleting an optional vanity option). No merge conflicts with #433.
|
Independent live re-validation on real nodogsplash 5.0.2 (PRTA virtual lab, OpenWrt 24.10.1 x86-64 VM, Debian client), plus one honest divergence from the earlier report and one edge-case finding. On-box results (fix deployed by running the PR's
Browser A/B recorded as video (pre/post), screenshots captured — artifacts on hand for the release thread. Honest divergence: on this lab's VM state I could not reproduce the infinite 307 form of the loop (curl or Chromium, even after NDS restart and re-setting the option fresh) — pre-fix I observe the self-referential Edge-case finding (minor, worth a follow-up line in this PR or an issue): the From our side: land — logic-verified, behavior-confirmed on the affected NDS version (again), with the edge case noted above as non-blocking. |
|
Browser A/B videos for the validation above (Chromium via Playwright against the virtual-lab router, same scenarios as the table):
Both scenarios navigate |
…es on same-version re-runs The #428 loop guard lived only in the full-setup path, so a device whose SETUP_VERSION already matches (VERSION has been v0.6.0-alpha2 since #380) exited at the wireless-verification early-return without ever reaching setup_nodogsplash's delete — postinst and the boot hook re-run the script, but same-version reinstalls never cleaned a stale option from a pre-#432 install. The early-exit path now deletes the option (and commits nodogsplash) when present, so 'actively deleted on upgrade' holds for every package operation, not just version-changing ones. CHANGELOG wording extended to match. sh -n and shellcheck -s sh clean (no new findings; the pre-existing busybox 'local' notes stand). Stock- device exposure wording corrected in the PR body per the review's upstream-init evidence; the hostname-on-non-fresh-device gap recorded as #444.
|
Review follow-ups landed in
Go side unchanged by this commit but re-verified: cc @felixfelix-bot — delete-order fixed with the claim now true everywhere it's made, wording honest, follow-ups filed; ready for another pass. |
|
#444 addressed on this branch (
Syntax-checked ( |
|
Correction: the previous comment pointed at |
|
#444 + two follow-up defects found and fixed by the live validation itself — all four brand scenarios now verified on the QEMU lab (OpenWrt 24.10.1, BusyBox ash, Scenarios (each: brand file → run the PR's setup script → assert):
The validation surfaced two real defects in the earlier commits, both fixed on this branch:
|
…-aware portal URL in both revert messages The #432 rework removed the originalDomain variable but left its final Printf referencing it — the CLI module did not compile (go vet: ssl.go: undefined: originalDomain). The farewell message now derives the portal URL from the system hostname like the rest of the brand-selection work; the self-signed revert's hardcoded TollGate.lan gets the same treatment so net4sats builds no longer print the wrong name.
…es on same-version re-runs The #428 loop guard lived only in the full-setup path, so a device whose SETUP_VERSION already matches (VERSION has been v0.6.0-alpha2 since #380) exited at the wireless-verification early-return without ever reaching setup_nodogsplash's delete — postinst and the boot hook re-run the script, but same-version reinstalls never cleaned a stale option from a pre-#432 install. The early-exit path now deletes the option (and commits nodogsplash) when present, so 'actively deleted on upgrade' holds for every package operation, not just version-changing ones. CHANGELOG wording extended to match. sh -n and shellcheck -s sh clean (no new findings; the pre-existing busybox 'local' notes stand). Stock- device exposure wording corrected in the PR body per the review's upstream-init evidence; the hostname-on-non-fresh-device gap recorded as #444.
dd87326 to
0fac209
Compare
…-aware portal URL in both revert messages The #432 rework removed the originalDomain variable but left its final Printf referencing it — the CLI module did not compile (go vet: ssl.go: undefined: originalDomain). The farewell message now derives the portal URL from the system hostname like the rest of the brand-selection work; the self-signed revert's hardcoded TollGate.lan gets the same treatment so net4sats builds no longer print the wrong name.
221802d to
2ad49f4
Compare
…es on same-version re-runs The #428 loop guard lived only in the full-setup path, so a device whose SETUP_VERSION already matches (VERSION has been v0.6.0-alpha2 since #380) exited at the wireless-verification early-return without ever reaching setup_nodogsplash's delete — postinst and the boot hook re-run the script, but same-version reinstalls never cleaned a stale option from a pre-#432 install. The early-exit path now deletes the option (and commits nodogsplash) when present, so 'actively deleted on upgrade' holds for every package operation, not just version-changing ones. CHANGELOG wording extended to match. sh -n and shellcheck -s sh clean (no new findings; the pre-existing busybox 'local' notes stand). Stock- device exposure wording corrected in the PR body per the review's upstream-init evidence; the hostname-on-non-fresh-device gap recorded as #444.
… redirect loop) nodogsplash 5.0.2 redirects every :2050 splash request that carries a redir param back to the splash itself when gatewaydomainname is configured, never issuing the client token — pre-auth clients loop until the browser aborts with ERR_TOO_MANY_REDIRECTS and never reach the payment page. Reproduced and control-grouped on the QEMU venue (with the option: 307 chain; without: splash serves 200) — see #428 for the probe transcript. The first-boot setup now actively deletes the option on upgrade; the friendly <hostname>.lan name keeps resolving because dnsmasq serves the system hostname in the lan zone (domain/expandhosts/local are already set by setup_dns_dnsmasq). tollgate ssl stops setting it as its step [4] too, and its revert cleans any value left by older installs instead of restoring the old hardcoded TollGate.lan default. Whitelabel hostname is brand-selected from a single /etc/tollgate/brand file ('tollgate', the default, or 'net4sats'; the two brands differ only in naming): the brand drives the system hostname and thus tollgate.lan / net4sats.lan DNS, the AP SSID prefix, and the NDS gateway name. Whitelabel installers write the file before first boot.
…-aware portal URL in both revert messages The #432 rework removed the originalDomain variable but left its final Printf referencing it — the CLI module did not compile (go vet: ssl.go: undefined: originalDomain). The farewell message now derives the portal URL from the system hostname like the rest of the brand-selection work; the self-signed revert's hardcoded TollGate.lan gets the same treatment so net4sats builds no longer print the wrong name.
…es on same-version re-runs The #428 loop guard lived only in the full-setup path, so a device whose SETUP_VERSION already matches (VERSION has been v0.6.0-alpha2 since #380) exited at the wireless-verification early-return without ever reaching setup_nodogsplash's delete — postinst and the boot hook re-run the script, but same-version reinstalls never cleaned a stale option from a pre-#432 install. The early-exit path now deletes the option (and commits nodogsplash) when present, so 'actively deleted on upgrade' holds for every package operation, not just version-changing ones. CHANGELOG wording extended to match. sh -n and shellcheck -s sh clean (no new findings; the pre-existing busybox 'local' notes stand). Stock- device exposure wording corrected in the PR body per the review's upstream-init evidence; the hostname-on-non-fresh-device gap recorded as #444.
…hostnames (#444) setup_hostname only assigned the brand hostname when the current value was exactly OpenWrt (fresh install), so a device switching /etc/tollgate/brand kept the old brand's hostname and a stale uhttpd commonname — Net4sats SSIDs advertising a TollGate hostname whose net4sats.lan never resolved. The hostname now migrates when the current value is a known brand default (OpenWrt/TollGate/Net4sats) and is preserved verbatim otherwise — operator-custom hostnames are never touched. The uhttpd commonname sync and the running-kernel apply are unchanged. Also logs when an unrecognized brand value coerces to the default (previously silent).
Live QEMU validation of the brand-switch scenario (#444) caught it: BusyBox tr treats '[:space:]' as a literal character set, deleting every s/p/a/c/e from the value — /etc/tollgate/brand=net4sats parsed as nt4t and silently coerced to the tollgate default, so the brand selector could never work on a real router. read -r strips the newline and IFS whitespace correctly on BusyBox ash (verified both with a bare value and one with a trailing space).
The brand selector ran at the top of the script, but both execution paths truncate /tmp/tollgate-setup.log (: > "$LOGFILE") after the version check — so the unrecognized-brand coercion line was written and then erased every run. The brand parsing now lives in load_brand(), called after each truncation point, before its first consumer. Live-validated on the QEMU lab (OpenWrt 24.10.1, BusyBox ash) across all four scenarios: brand-default switch in both directions migrates hostname+commonname; a custom hostname is never touched (even with a typo'd brand); the coercion line survives in the log.
2ad49f4 to
975297d
Compare
What
Fixes the captive-portal redirect loop on nodogsplash 5.0.2 (#428) and makes the whitelabel hostname a single brand selector.
:2050/splash.htmlrequest carrying aredirparam with307 → …splash.html?redir=<itself>whengatewaydomainnameis configured — pre-auth clients (phones triggering the captive portal) loop until the browser gives up withERR_TOO_MANY_REDIRECTSand never see the payment page. Our first-boot setup sets that option, so every current install is affected. The setup now actively deletes it on upgrade; the friendly<hostname>.lanname keeps working because dnsmasq already serves the system hostname in thelanzone.tollgate ssl: its step [4] set the same option (and revert restored a hardcodedTollGate.lan) — removed and cleaned up on revert; the TLS flow's dnsmasq entry is what makes the cert domain resolve./etc/tollgate/brand(tollgate, the default, ornet4sats— the brands differ only in naming) now drives the system hostname (thustollgate.lan/net4sats.lanDNS), AP SSID prefix, and NDS gateway name. Whitelabel installers write the file before first boot; absent file = default brand, so existing installs are unchanged.Verification
gofmt/go vet/go buildgreen;sh -non the uci-defaults.Authenticatedinndsctl→ internet through the gate (see the control-group probes in nodogsplash 5.0.2: gatewaydomainname makes the pre-auth splash redirect-loop (ERR_TOO_MANY_REDIRECTS) #428). Recorded happy paths for both brands after the fix:evidence/phone-demo/on physical-router-test-automation#144's lane — TollGate-branded and net4sats-whitelabel (Net4satshostname /net4sats.lan/ net4sats portal build) both reach "You're connected!".Fixes #428