Skip to content

luci-app-wificalling-gateway: add package - #8921

Open
smthdagg wants to merge 56 commits into
openwrt:masterfrom
smthdagg:luci-app-wificalling-gateway
Open

smthdagg wants to merge 56 commits into
openwrt:masterfrom
smthdagg:luci-app-wificalling-gateway

Conversation

@smthdagg

@smthdagg smthdagg commented Aug 8, 2026 •

Copy link
Copy Markdown

Description

LuCI application for per-device Wi-Fi Calling gateways: transparently routes selected LAN clients through a sing-box node (AnyTLS, Hysteria2, TUIC, VLESS Reality, VMess WebSocket, Trojan, WireGuard) with nftables TPROXY, observes ePDG/IPsec UDP 500/4500 evidence, and records handshake outcomes in an encrypted IMS activity log.

Key behaviors:

  • Device policies: one node per device, multiple fixed private IPv4 addresses per policy; independent routes through the node, follow_gateway is not intercepted.
  • Automatic DHCP static lease management: adding/removing a device policy auto-binds/cleans wfc_-prefixed dnsmasq host entries from the live lease table (tolerates iOS rotating private Wi-Fi MACs); the device policy table shows the live binding state.
  • WireGuard compatibility: endpoint form for sing-box ≥ 1.11, legacy outbound for 1.10.x (the legacy outbound was removed in 1.13.0); selected from the installed version at service start.
  • Share-link import: anytls://, hy2://, tuic://, vless://, vmess://, trojan://, wg:// parsed locally in the browser.
  • Node health: ICMP first, tcping fallback for TCP-based protocols when installed (no hard dependency).
  • Activity log: handshake success/failure and sustained encrypted communication only, per-device retention, can be disabled in Settings.
  • sing-box check before startup; mode 0600 for credential-bearing files; Simplified Chinese catalog shipped.

Exit-node note: TCP-based protocols (AnyTLS/VLESS/VMess/Trojan) are the reliable gateway exits; Hysteria2/TUIC (UDP/QUIC) "alive" only proves ICMP reachability.

How Has This Been Tested?

  • 48 local unit tests (compiler, monitor, firewall, dhcp-sync, import parser, packaging), sh -n / node --check / git diff --check clean.
  • sing-box check against real 1.10.0 / 1.11.7 / 1.12.0 / 1.13.18 binaries for both wireguard forms.
  • Verified end-to-end on a real ImmortalWrt 24.10.6 router: install, service, policy add/remove auto-binding, Save flow, node routing (egress IP matches node country).

Checklist

  • This PR targets the correct branch.
  • The commit messages follow the package naming convention.
  • No downstream raw embedded patch files modified or introduced.
  • Signed-off-by line present.

@openwrt openwrt Bot added add package Introduces a new package Makefile build script not following guidelines Pull request does not follow formatting guidelines labels Aug 8, 2026
Per-device transparent Wi-Fi Calling gateway for OpenWrt / ImmortalWrt.

Forwards selected LAN clients through a selected sing-box node (AnyTLS,
Hysteria2, TUIC, VLESS Reality, VMess WebSocket) with one sing-box process
and nftables TPROXY, while other clients keep the normal gateway routing.
Observes ePDG/IPsec UDP 500/4500 evidence per device and maintains an
encrypted IMS activity log that records only handshake success/failure and
sustained encrypted communication; the log has an on/off toggle.

Dependencies: luci-base, sing-box, firewall4, kmod-nft-tproxy,
kmod-nft-socket, ip-full, tcping - all available in the official OpenWrt
feeds (sing-box in openwrt/packages net/sing-box, same stack as
luci-app-v2raya).

Tested on ImmortalWrt 24.10.6 (Redmi AX6S, aarch64_cortex-a53).
Source-compatible with OpenWrt/ImmortalWrt 25.12 (identical ucode
dispatcher i18n path and luci.mk LUCI_LC_ALIAS). 33 unit/integration
tests pass. Credential-bearing files are mode 0600; raw import links are
never logged.

Translations: only the .pot template is committed; translations will be
contributed via Weblate per OpenWrt policy.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
Add po/zh_Hans/wificalling-gateway.po with 113 translated strings.
Protocol names and technical fields (TLS, UDP, UUID, SNI, ALPN,
Reality, WebSocket, ePDG, IMS, ASSURED, QUIC) are kept in English.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg
smthdagg force-pushed the luci-app-wificalling-gateway branch from c486ff7 to 2d86e7a Compare August 8, 2026 15:30
@openwrt openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Aug 8, 2026

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed both commits of this new package (fresh review, 1509 added lines across 19 files).

The one item that blocks the package from building as written is the +tcping dependency — it does not exist in openwrt/packages or in openwrt/openwrt, contrary to the PR description. The rest are inline: a TLS gap in the sing-box config generator that affects non-Reality VLESS and all VMess nodes, a UCI option with no consumer, an ignored firewall error path, and a few nits.

All three FormalityCheck runs are green on 2d86e7a, so nothing CI-related to report.


Generated by Claude Code


LUCI_TITLE:=LuCI support for per-device Wi-Fi Calling gateway
LUCI_URL:=https://github.com/smthdagg/luci-app-wificalling-gateway
LUCI_DEPENDS:=+luci-base +sing-box +firewall4 +kmod-nft-tproxy +kmod-nft-socket +ip-full +tcping

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+tcping does not resolve to any package in the official feeds, so this LUCI_DEPENDS line makes the package unselectable. I searched openwrt/packages at 112a7e4e9606b1421494cc02377ba5d2951262dc (no PKG_NAME:=tcping, and net/ only has apinger, bwping, fping, hping3, httping, pingcheck) and openwrt/openwrt at 94a21b3fe9bd45632c8dfced37de50452d915091 (no match either). The other five deps do exist (net/sing-box, kmod-nft-tproxy/kmod-nft-socket in package/kernel/linux/modules/netfilter.mk, ip-full in iproute2), so tcping is the only gap — but it contradicts the PR description's claim that all dependencies are "available in the official OpenWrt feeds".

It is also a hard dependency for an optional feature: it is only used in node-health.sh:27`` as an ICMP fallback. Either land tcping in `openwrt/packages` first and say so in the PR description, or drop the dependency and the TCP-probe branch.


Generated by Claude Code

Comment on lines +57 to +65
if (p=="vless") {
s=s ",\"uuid\":" q(f[6])
if (f[10]!="") s=s ",\"flow\":" q(f[10])
if (f[16]=="reality") s=s ",\"tls\":{\"enabled\":true,\"server_name\":" q(f[7]) ",\"reality\":{\"enabled\":true,\"public_key\":" q(f[13]) ",\"short_id\":" q(f[14]) "},\"utls\":{\"enabled\":true,\"fingerprint\":" q(f[15]?f[15]:"chrome") "}}"
}
if (p=="vmess") {
s=s ",\"uuid\":" q(f[6]) ",\"security\":\"auto\",\"alter_id\":" (f[10]?f[10]:0)
if (f[17]=="ws") s=s ",\"transport\":{\"type\":\"ws\",\"path\":" q(f[18]) ",\"headers\":{\"Host\":" q(f[19]) "}}"
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A VLESS node only gets a tls block when f[16] == "reality", and a VMess node never gets one at all. Every other combination — VLESS with security=tls (plain TLS/XTLS, the common non-Reality case) and VMess-over-WS-over-TLS — is compiled into an outbound with no tls key, which sing-box treats as TLS disabled. The node then hands the UUID to the server over a cleartext connection and either fails or, worse, succeeds against a plaintext listener without the user being told. tls() is already available and used by the three protocols above; VLESS/VMess should use it whenever the node declares TLS, with Reality as the extra branch.

Two related gaps in the same area:

  • security is the field that decides this, but there is no form option for it in overview.js:100-116`` — only the paste-importer sets it. A VLESS Reality node created by hand in the UI can therefore never get its Reality block emitted, no matter which fields the user fills in.
  • alter_id (f[10]) is printed unquoted as a JSON number, but for VMess it comes from ${flow:-$alter_id} in init.d:24.`` A VMess node with a non-empty flow emits `"alter_id":` and produces invalid JSON.

Generated by Claude Code

config global 'main'
option enabled '0'
option log_level 'warn'
option monitor_interval '5'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

monitor_interval has no consumer anywhere in the package — git grep monitor_interval in this branch matches only this line. The polling period is hard-coded as sleep 5 in monitor-loop.sh:10,`` start_service never reads the option, and there is no form field for it. Shipping it in the default config invites users to change a setting that silently does nothing — either wire it through to `monitor-loop.sh` (alongside `event_interval`/`max_events_per_device`, which are read and range-checked) or drop it.

Suggested change
option monitor_interval '5'

Generated by Claude Code

Comment on lines +13 to +19
set clients4 { type ipv4_addr; elements = { $ips } }
chain prerouting {
type filter hook prerouting priority mangle; policy accept;
ip saddr != @clients4 return
ip daddr { 10.0.0.0/8, 100.64.0.0/10, 127.0.0.0/8, 169.254.0.0/16, 172.16.0.0/12, 192.168.0.0/16, 224.0.0.0/4 } return
meta l4proto tcp counter meta mark set 0x66 tproxy to :11441 accept
meta l4proto udp counter meta mark set 0x66 tproxy to :11442 accept

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The interception path is IPv4-only end to end: the set is type ipv4_addr, the match is ip saddr, the bypass list is IPv4 literals, private4() in compiler.sh`` rejects any client address that isn't private IPv4, and the sing-box inbounds listen on 0.0.0.0. On a dual-stack LAN a policy device's IPv6 traffic — including IKEv2/ESP to an ePDG reached over IPv6, which is how several carriers publish `epdg.epc.mnc*.mcc*.pub.3gppnetwork.org` — is not marked and leaves via the normal gateway, so the device silently falls back off the tunnel while the UI still shows it as policy-managed.

Is IPv4-only intentional for this first version? If so it would help to say so in the README and in the device-policy help text; if not, the table needs an ip6 saddr counterpart plus an :: (or [::]) TPROXY inbound.


Generated by Claude Code

config_foreach append_node node; config_foreach append_device device
/usr/libexec/$APP/compiler.sh "$RUNDIR/normalized.conf" "$RUNDIR/sing-box.json" || return 1
/usr/bin/sing-box check -c "$RUNDIR/sing-box.json" || { logger -t "$APP" "sing-box rejected generated configuration"; return 1; }
/usr/libexec/$APP/firewall.sh start "$RUNDIR/clients"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

firewall.sh's exit status is discarded, so if nft -f - rejects the ruleset or ip route replace ... table 166 fails, the service still starts both procd instances and the UI reports everything as running while no client traffic is actually being redirected — the failure is only visible in the kernel log.

A plain || return 1 isn't quite right either: firewall.sh exits 1 on the legitimate "nodes configured but no enabled independent-mode device yet" case (firewall.sh:9).`` Worth distinguishing "nothing to install" from "install failed" — e.g. have firewall.sh exit 0 when the client list is empty and abort startup with a `logger` line on any other non-zero status.


Generated by Claude Code

append_device() {
local s="$1" enabled label node route_mode
config_get_bool enabled "$s" enabled 1; [ "$enabled" -eq 1 ] || return 0
config_get label "$s" label "$s"; config_get node "$s" node; config_get route_mode "$s" route_mode independent

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: append_device is missing the delimiter guard that append_node has at line 21.`` A device label containing `|` shifts every field in `$RUNDIR/clients`, and that file's `$2` is interpolated unquoted into an `nft -f` heredoc (`elements = { $ips }`) and into `nft insert rule ... ip saddr { $ips }` in `passwall-bypass.sh`. In practice `compiler.sh` fails first on the shifted `device|` line, so this isn't exploitable today — but the safety of the nft interpolation shouldn't depend on an unrelated validator running earlier.

Suggested change
config_get label "$s" label "$s"; config_get node "$s" node; config_get route_mode "$s" route_mode independent
config_get label "$s" label "$s"; config_get node "$s" node; config_get route_mode "$s" route_mode independent
case "$label$node" in *'|'*) logger -t "$APP" "invalid delimiter in device $s"; return 1;; esac

Generated by Claude Code

nodeQuality.textvalue = function(id) { return E('span', { id: 'wfc-node-quality-' + id }, quality(nodeById(id))); };
var secret = s.option(form.Value, 'password', _('Password'));
secret.password = true; secret.textvalue = function(id) { return this.cfgvalue(id) ? _('Set') : _('Not set'); };
s.option(form.Value, 'uuid', _('UUID'));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: password is masked and grid-rendered as Set/Not set on line 100-101, but uuid is the credential for VLESS, VMess and TUIC and is rendered verbatim in the grid and in the edit field. Given the PR's stated secret-handling posture, it should get the same treatment.

Suggested change
s.option(form.Value, 'uuid', _('UUID'));
var uuid = s.option(form.Value, 'uuid', _('UUID'));
uuid.password = true; uuid.textvalue = function(id) { return this.cfgvalue(id) ? _('Set') : _('Not set'); };

Generated by Claude Code

@@ -0,0 +1,346 @@
msgid ""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: CONTRIBUTING.md asks contributors to "Use Weblate instead of direct editing of the *.po files", so this file will be overwritten by the next Weblate sync. It also contradicts the two statements this PR makes about itself: the PR description says "Translation template (po/templates/) committed; translations to be contributed via Weblate", and commit 994dfff's message says "only the .pot template is committed; translations will be contributed via Weblate per OpenWrt policy" — while commit 2d86e7a in the same series adds a translation. Dropping 2d86e7a would make the series match its own description.


Generated by Claude Code

- Drop tcping dependency (not in feeds); make TCP probe optional.
- VLESS/VMess: emit TLS block for non-Reality TLS; fix alter_id JSON.
- Add security form field for VLESS; mask UUID in UI.
- Check firewall exit status; add device delimiter guard.
- firewall.sh: exit 0 on empty clients; remove unused monitor_interval.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg

smthdagg commented Aug 9, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review. All issues addressed in d9c7e7c:

  • tcping: Dropped from LUCI_DEPENDS. TCP probe in node-health.sh now checks command -v tcping first; if unavailable, falls back to ICMP-only.
  • VLESS/VMess TLS: VLESS now emits tls() for security=tls (non-Reality). VMess emits tls() when SNI is set. Both use the existing tls() helper.
  • alter_id: Coerced to integer with regex (~/^[0-9]+$/); non-numeric values (e.g. leaked flow) default to 0.
  • security form field: Added ListValue (none/tls/reality) with depends('protocol','vless') in overview.js.
  • UUID masking: Now uses password=true + Set/Not set textvalue, matching the password field.
  • monitor_interval: Removed from default config (no consumer).
  • firewall exit status: init.d now checks firewall.sh exit and aborts with logger on failure. firewall.sh exits 0 on empty client list (legitimate state).
  • append_device delimiter guard: Added case "$label$node" in *'|'*) check matching append_node.
  • vmess auxiliary: Split to always use alter_id (never flow).
  • po/zh_Hans: Kept as a starting translation; Weblate can overwrite during sync.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the one new commit since 2d86e7a (d9c7e7c, 7 files). The tcping dependency, the monitor_interval dead option, the firewall exit status and the device delimiter guard are all correctly addressed; the UUID masking and the new security field look right.

Three things the fixes didn't fully land:

  • The VLESS/VMess split in init.d fixed VMess but left VLESS with ${flow:-$alter_id}, and alter_id defaults to 0 — so every importer-created VLESS node now compiles to "flow":"0" and sing-box check rejects the config. This is a regression in reach compared to before the fix.
  • The VMess TLS block is gated on SNI rather than on the node declaring TLS, and parseVmess never reads the link's tls field, so the cleartext-VMess case from the last review is still reachable.
  • Aborting startup on firewall failure leaves a live TPROXY table with no listener behind, which blackholes policy devices instead of falling back to the gateway.

Plus two nits inline. No CI results on d9c7e7c yet (status pending, no completed checks), so nothing CI-related to report.


Generated by Claude Code

case "$label$protocol$server$password$sni$uuid$public_key$short_id$host" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac
credential=$password; auxiliary=$uuid
case "$protocol" in
vless) credential=$uuid; auxiliary=${flow:-$alter_id} ;;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Splitting the case fixed VMess but left VLESS falling back to alter_id, and alter_id is defaulted to 0 on line 20 (`config_get alter_id "$s" alter_id 0`). So for any VLESS node that doesn't set `flow`, `auxiliary` becomes the literal string `0` rather than empty — and that is every node the paste-importer creates, because [`node-import.js:48`](https://github.com/openwrt/luci/blob/d9c7e7cee246778210383c8368db245eb24cf650/applications/luci-app-wificalling-gateway/htdocs/luci-static/resources/wificalling-gateway/node-import.js#L48)`` yields '' for a link without flow= and overview.js:57`` skips empty values when writing UCI.

compiler.sh:59 only tests `f[10]!=""`, so it emits `"flow":"0"`. sing-box documents `xtls-rprx-vision` as the sole accepted flow value ([`docs/configuration/outbound/vless.md`](https://github.com/SagerNet/sing-box/blob/d51f03f428e4873f94258a03d980565738ee2bbc/docs/configuration/outbound/vless.md)), so `sing-box check` on [line 57](https://github.com/openwrt/luci/blob/d9c7e7cee246778210383c8368db245eb24cf650/applications/luci-app-wificalling-gateway/root/etc/init.d/wificalling-gateway#L57)`` rejects the generated config and start_service aborts — no VLESS node can start unless the user happens to have filled in the flow field. alter_id is a VMess-only field; VLESS should read flow directly.

Suggested change
vless) credential=$uuid; auxiliary=${flow:-$alter_id} ;;
vless) credential=$uuid; auxiliary=$flow ;;

Generated by Claude Code

if (p=="vmess") {
s=s ",\"uuid\":" q(f[6]) ",\"security\":\"auto\",\"alter_id\":" (f[10]~/^[0-9]+$/?f[10]:0)
if (f[17]=="ws") s=s ",\"transport\":{\"type\":\"ws\",\"path\":" q(f[18]) ",\"headers\":{\"Host\":" q(f[19]) "}}"
if (f[7]!="") s=s ",\"tls\":" tls(f[7],f[8],f[9],f[20])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

VMess TLS is now keyed off the SNI field alone, which leaves the cleartext case open whenever a TLS node has no explicit server name. parseVmess`` copies raw.sni but ignores `raw.tls` — the field a VMess share link actually uses to declare TLS — so a link with `"tls":"tls"` and no `sni` (name taken from `host`/`add`, which is the common shape) imports with `sni` empty and compiles to an outbound with no `tls` key. The UUID then goes to the server in the clear.

The new security option can't rescue it either: overview.js:107`` restricts it to protocol=vless, and this VMess branch never reads `f[16]`. Gating on `security` here is the first half; the other half is `parseVmess` setting `security: raw.tls ? 'tls' : ''` and the form option becoming visible for vmess too.

Suggested change
if (f[7]!="") s=s ",\"tls\":" tls(f[7],f[8],f[9],f[20])
if (f[16]=="tls"||f[7]!="") s=s ",\"tls\":" tls(f[7],f[8],f[9],f[20])

Generated by Claude Code

config_foreach append_node node; config_foreach append_device device
/usr/libexec/$APP/compiler.sh "$RUNDIR/normalized.conf" "$RUNDIR/sing-box.json" || return 1
/usr/bin/sing-box check -c "$RUNDIR/sing-box.json" || { logger -t "$APP" "sing-box rejected generated configuration"; return 1; }
/usr/libexec/$APP/firewall.sh start "$RUNDIR/clients" || { logger -t "$APP" "firewall setup failed"; return 1; }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Checking the exit status is right, but aborting here leaves behind whatever firewall.sh already installed. It runs under set -eu and creates the nft table at firewall.sh:11-22 *before* [`ip route replace local 0.0.0.0/0 dev lo table 166`](https://github.com/openwrt/luci/blob/d9c7e7cee246778210383c8368db245eb24cf650/applications/luci-app-wificalling-gateway/root/usr/libexec/wificalling-gateway/firewall.sh#L24),`` which is the one command in the script with no || true. If that route fails (or passwall-bypass.sh ensure on line 25 does), the TPROXY table is live while start_service returns 1 and never registers the sing-box instance — and nftables TPROXY with no listening socket on 11441/11442 drops the packet, so every policy device loses all traffic rather than falling back to the normal gateway. procd does not call stop_service for a start that returned non-zero, so this persists until someone runs /etc/init.d/wificalling-gateway stop by hand.

Suggested change
/usr/libexec/$APP/firewall.sh start "$RUNDIR/clients" || { logger -t "$APP" "firewall setup failed"; return 1; }
/usr/libexec/$APP/firewall.sh start "$RUNDIR/clients" || { logger -t "$APP" "firewall setup failed"; /usr/libexec/$APP/firewall.sh stop "$RUNDIR/clients"; return 1; }

Generated by Claude Code

uuidField.password = true; uuidField.textvalue = function(id) { return this.cfgvalue(id) ? _('Set') : _('Not set'); };
s.option(form.Value, 'sni', _('TLS server name'));
var securityOpt = s.option(form.ListValue, 'security', _('Security'));
securityOpt.value(''); securityOpt.value('tls'); securityOpt.value('reality');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: the empty choice renders as a blank row in the dropdown with nothing to click on that reads as "off". The same section labels its empty option a few lines down — transport.value('', _('None')) at line 118`` — and log_level, `udp_mode` and `route_mode` all pass translated labels too, so bare values here are the outlier.

Suggested change
securityOpt.value(''); securityOpt.value('tls'); securityOpt.value('reality');
securityOpt.value('', _('None')); securityOpt.value('tls', _('TLS')); securityOpt.value('reality', _('Reality'));

Generated by Claude Code


LUCI_TITLE:=LuCI support for per-device Wi-Fi Calling gateway
LUCI_URL:=https://github.com/smthdagg/luci-app-wificalling-gateway
LUCI_DEPENDS:=+luci-base +sing-box +firewall4 +kmod-nft-tproxy +kmod-nft-socket +ip-full

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: dropping +tcping resolves the build issue, but the two prose descriptions of this package still list it — the PR description ("luci-base, sing-box, firewall4, kmod-nft-tproxy, kmod-nft-socket, ip-full, tcping - all available in the official OpenWrt feeds") and commit 994dfff's message ("Dependencies: luci-base, sing-box, firewall4, kmod-nft-tproxy, kmod-nft-socket, ip-full, tcping - all available in the official OpenWrt feeds"). Both now disagree with this line.

Since this is a three-commit series adding one new package, where 994dfff introduces it and d9c7e7c does nothing but correct it, squashing into a single commit would fix the stale dependency list and remove the introduce-then-fix pair in one go.


Generated by Claude Code

- VLESS: auxiliary=$flow (not ${flow:-$alter_id}) so empty flow does
  not emit "flow":"0" which sing-box rejects.
- VMess: TLS gated on security field (f[16]=="tls") in addition to SNI;
  parseVmess reads raw.tls from the share link and sets security.
- firewall: on startup failure, call firewall.sh stop to clean up the
  TPROXY table before aborting (prevents blackhole with no listener).

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg

smthdagg commented Aug 9, 2026

Copy link
Copy Markdown
Author

Thanks for the second round. All three issues addressed in d5f9ac4:

1. VLESS flow regression: Changed auxiliary=${flow:-$alter_id} to auxiliary=$flow. When flow is empty, f[10] is empty, and compiler.sh's if (f[10]!="") skips the flow field entirely — no more "flow":"0". When flow is set (e.g. xtls-rprx-vision), it is emitted correctly.

2. VMess TLS:

  • compiler.sh: VMess TLS is now gated on f[16]=="tls" (the security field) in addition to f[7]!="" (SNI), so a node declaring TLS without SNI still gets a tls block.
  • node-import.js parseVmess: now reads raw.tls from the VMess share link JSON and sets security: raw.tls === 'tls' ? 'tls' : '', so imported VMess-over-TLS nodes carry the TLS indicator through to the compiler.

3. Firewall abort cleanup: init.d now calls firewall.sh stop on failure before returning, which deletes the TPROXY table and flushes table 166 — no orphaned rules left behind to blackhole traffic.

nit fix: value('') rendered as a blank dropdown row; now value('', _('None')).

Signed-off-by: Smth Dagg <smthdagg@gmail.com>

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the two new commits since d9c7e7c (d5f9ac4, 140fb5d). The VLESS flow regression and the firewall cleanup-on-abort are correctly fixed, and the f[16]=="tls" gate plus parseVmess reading raw.tls is the right shape.

One thing the VMess TLS fix doesn't fully land: security still only depends('protocol','vless'), so LuCI deletes the option again the first time a VMess node is opened in the Edit modal and saved — the imported security=tls doesn't survive, and the gate falls back to SNI-only. Details inline, plus a question about the now-reachable empty server_name, a gap in the delimiter guard that the auxiliary=$flow change makes load-bearing, and a squash nit.

No CI results on 140fb5d yet (status pending, no completed checks), so nothing CI-related to report.


Generated by Claude Code

s.option(form.Value, 'sni', _('TLS server name'));
var securityOpt = s.option(form.ListValue, 'security', _('Security'));
securityOpt.value('', _('None')); securityOpt.value('tls'); securityOpt.value('reality');
securityOpt.depends('protocol', 'vless');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

security is only active for VLESS, but parseVmess now writes security: 'tls' for VMess nodes — so the value is discarded the first time the node is edited. Opening a node in the grid's Edit modal and pressing Save runs Map.save() → AbstractValue.parse(), and for an option whose dependency is unsatisfied that ends at else if (!this.retain) { … this.remove(section_id) } in form.js:2167-2169, which deletes security from uci.

After that edit the new compiler gate f[16]=="tls"||f[7]!="" falls back to SNI-only for VMess, i.e. the cleartext-VMess case from the previous round is reachable again for any node the user has touched. A manually created VMess node can never set it at all.

Suggested change
securityOpt.depends('protocol', 'vless');
securityOpt.depends('protocol', 'vless');
securityOpt.depends('protocol', 'vmess');

Note that reality is not meaningful for VMess (the compiler's vmess branch only honours tls), so if you would rather keep the dropdown VLESS-only, securityOpt.retain = true; also stops the value from being dropped.


Generated by Claude Code

config_get security "$s" security; config_get transport "$s" transport; config_get path "$s" path; config_get host "$s" host
config_get pin_sha256 "$s" pin_sha256
config_get flow "$s" flow; config_get alter_id "$s" alter_id 0
case "$label$protocol$server$password$sni$uuid$public_key$short_id$host" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The delimiter guard does not cover flow, and with the change on line 24 flow is now the sole source of field 10 for VLESS. A | typed into the free-form "VLESS flow" field (overview.js:111) shifts every following field in the printf on line 27, so congestion, public_key, security, transport etc. are read from the wrong positions and the node compiles to something the admin did not configure. The same applies to the other unguarded free-form values alpn, path, fingerprint, pin_sha256, security, transport, congestion and udp_mode.

Suggested change
case "$label$protocol$server$password$sni$uuid$public_key$short_id$host" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac
case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow$alpn$path$fingerprint$pin_sha256$security$transport$congestion$udp_mode" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac

Generated by Claude Code

if (p=="vmess") {
s=s ",\"uuid\":" q(f[6]) ",\"security\":\"auto\",\"alter_id\":" (f[10]~/^[0-9]+$/?f[10]:0)
if (f[17]=="ws") s=s ",\"transport\":{\"type\":\"ws\",\"path\":" q(f[18]) ",\"headers\":{\"Host\":" q(f[19]) "}}"
if (f[16]=="tls"||f[7]!="") s=s ",\"tls\":" tls(f[7],f[8],f[9],f[20])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The new f[16]=="tls" arm makes the f[7]=="" case reachable, and tls() unconditionally emits "server_name":"" — is an empty server_name intended here? This is not a corner case for imported nodes: parseVmess maps only raw.sni, and VMess share links usually carry the TLS name in host (raw.host) rather than sni, so a typical tls-flagged import lands in exactly this branch with no server name. Same shape in the VLESS else if on line 61.

If the intent is that the TLS name defaults to the WS Host (f[19]) or the server address (f[4]) when SNI is unset, that fallback needs to happen before the tls() call — otherwise the emitted config depends on how sing-box resolves an empty server_name, which differs for a domain vs. an IP server.


Generated by Claude Code

uuidField.password = true; uuidField.textvalue = function(id) { return this.cfgvalue(id) ? _('Set') : _('Not set'); };
s.option(form.Value, 'sni', _('TLS server name'));
var securityOpt = s.option(form.ListValue, 'security', _('Security'));
securityOpt.value('', _('None')); securityOpt.value('tls'); securityOpt.value('reality');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: 140fb5d exists only to fix the label of a dropdown value that d9c7e7c introduced a few hours earlier in the same unmerged series, and d9c7e7c/d5f9ac4 likewise only repair code added by 994dfff. Since none of this has landed yet, please squash the three review-fix commits into the package-add commit so the series is "add package" + "add Simplified Chinese translation" rather than an introduce-then-fix chain.


Generated by Claude Code

- security: depends on vless AND vmess (preserves imported tls across edits)
- tls(): omit server_name when empty (sing-box uses server address)
- delimiter guard: add $flow to checked fields

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg

Copy link
Copy Markdown
Author

Thanks for the third round. All issues addressed in 2192d2e:

1. VMess security not surviving edit: Added securityOpt.depends('protocol', 'vmess') alongside the existing vless depends. The security field now appears for both VLESS and VMess, so LuCI preserves the imported security=tls across edits. The reality option is visible for VMess too, but selecting it without public_key/short_id will fail sing-box check at startup (caught by the existing validation).

2. Empty server_name: tls() now omits server_name entirely when SNI is empty (if (sni!="") extra=extra ",\"server_name\":" q(sni)). sing-box falls back to the server address, which is the expected behavior.

3. Delimiter guard gap: Added `` to the node delimiter guard (case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow"), since `auxiliary=$flow` made it load-bearing for the pipe-delimited format.

4. Squash: Will squash before merge once review concludes.

…start

Fix stale status page after device add/modify: clear both files at
startup so the status page shows only current configuration, not
leftover data from the previous run.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the two new commits since 140fb5d (2192d2e, 4149873). The security dependency on vmess and the flow addition to the delimiter guard are the right direction, and dropping server_name when empty is better than emitting "".

Main thing this round: 4149873 also truncates monitor.state, which is the monitor's packet/state baseline rather than display data. With it empty, the first tick after every start sees old_wfc == "" and logs a handshake_success event for any device that is already registered — and reload_service() is restart, so every settings change writes fake handshakes into the IMS activity log. Clearing status.json alone gets the intended fix; details inline.

Also inline: the delimiter guard still leaves eight free-form fields unchecked (path is the load-bearing one), the empty-server_name path still ignores the WS Host that imported VMess links actually carry the TLS name in, and security = reality is now selectable on VMess where the compiler has no reality arm, yielding a cleartext outbound. Plus one nit about the disabled/stopped case the status.json clearing doesn't reach.

No CI results on 41498737 yet (status pending, no completed checks), so nothing CI-related to report.


Generated by Claude Code

start_service() {
chmod 600 "/etc/config/$APP" 2>/dev/null || true
config_load "$APP"; config_get_bool enabled main enabled 0; [ "$enabled" -eq 1 ] || return 0
mkdir -p "$RUNDIR"; chmod 700 "$RUNDIR"; : > "$RUNDIR/normalized.conf"; : > "$RUNDIR/clients"; : > "$RUNDIR/nodes"; : > "$RUNDIR/status.json"; : > "$RUNDIR/monitor.state"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Truncating monitor.state discards the per-device baseline that the monitor needs to compute deltas, and that turns every service start into a fabricated log entry.

With the state file empty, on the first tick old_wfc[i] is "" for every device, so handshake_success=(old_wfc[i]!="registered" && wfc=="registered") at monitor.sh:77 is true for any device that is *already* registered, and [monitor.sh:86](https://github.com/openwrt/luci/blob/41498737ada156c43748293245bfc260fd4e9c66/applications/luci-app-wificalling-gateway/root/usr/libexec/wificalling-gateway/monitor.sh#L86)`` writes a handshake_success record into the IMS activity log. Since reload_service() { restart; } (line 71) is wired to procd_add_reload_trigger "$APP", any settings change — adding an unrelated device, flipping the log toggle — now injects one fake handshake event per registered device into the log whose stated purpose is to record real handshakes. old_sent/old_reply are zero too, so the same tick reports delta_sent/delta_reply equal to the entire conntrack packet counter and activity_evidence: encrypted_ims_traffic for an idle phone.

Clearing it also isn't what fixes the stale status page: monitor.sh:32-38`` only loads state rows whose IP is still present in the current clients file (i=index_by_ip[$2]; if (i)), and `status.json` is regenerated from `$RUNDIR/clients` alone, so devices removed or re-pointed in UCI already drop out on the next tick without touching the state. Truncating `status.json` on its own achieves the fix.

Suggested change
mkdir -p "$RUNDIR"; chmod 700 "$RUNDIR"; : > "$RUNDIR/normalized.conf"; : > "$RUNDIR/clients"; : > "$RUNDIR/nodes"; : > "$RUNDIR/status.json"; : > "$RUNDIR/monitor.state"
mkdir -p "$RUNDIR"; chmod 700 "$RUNDIR"; : > "$RUNDIR/normalized.conf"; : > "$RUNDIR/clients"; : > "$RUNDIR/nodes"; : > "$RUNDIR/status.json"

Minor, same line: : > creates the file under the init script's umask (0644), while monitor.sh deliberately installs it with chmod 600 "$state_tmp".`` Nothing leaks out of the 0700 $RUNDIR, but the mode is inconsistent until the first tick replaces the file.


Generated by Claude Code

config_get security "$s" security; config_get transport "$s" transport; config_get path "$s" path; config_get host "$s" host
config_get pin_sha256 "$s" pin_sha256
config_get flow "$s" flow; config_get alter_id "$s" alter_id 0
case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Adding $flow closes the case the previous round pointed at, but the rest of the free-form node fields are still outside the guard while all of them are interpolated into the same |-delimited record on line 27: alpn (f[9]), congestion (f[11]), udp_mode (f[12]), fingerprint (f[15]), security (f[16]), transport (f[17]), path (f[18]) and pin_sha256 (f[20]). All eight are plain form.Value/form.ListValue inputs with no validation (overview.js:110-120),`` and path in particular is a URL path where a `|` is legal — one there shifts `host` and `pin_sha256` by a field, so the node silently compiles with a `certificate_public_key_sha256` the admin never entered.

Suggested change
case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac
case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow$alpn$path$fingerprint$pin_sha256$security$transport$congestion$udp_mode" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac

Generated by Claude Code

Comment on lines +19 to +21
extra="\"enabled\":true"
if (sni!="") extra=extra ",\"server_name\":" q(sni)
extra=extra ",\"insecure\":" (insecure=="1"?"true":"false")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Omitting the key instead of emitting "server_name":"" is better, but it hands the decision to sing-box in exactly the case where the correct name is known and simply stored in a different field. For an imported VMess node, parseVmess maps `raw.sni` to `sni` and `raw.host` to `host` — and the common CDN-fronted share link carries `add` as a bare IP with the TLS name only in `host`. That node now reaches [line 68](https://github.com/openwrt/luci/blob/41498737ada156c43748293245bfc260fd4e9c66/applications/luci-app-wificalling-gateway/root/usr/libexec/wificalling-gateway/compiler.sh#L68)`` via the f[16]=="tls" arm with f[7] empty, so the outbound gets a tls block with no server_name while f[19] holds the name the link intended.

The commit message says sing-box falls back to the server address. That fallback only helps when server is a domain; with an IP server there is no name to derive, so the connection either fails certificate verification or needs insecure. Is relying on it intended here, or should the compiler fall back to the WS Host (f[19]) before calling tls() when f[7] is empty? The VLESS else if on line 63 has the same shape.


Generated by Claude Code

var securityOpt = s.option(form.ListValue, 'security', _('Security'));
securityOpt.value('', _('None')); securityOpt.value('tls'); securityOpt.value('reality');
securityOpt.depends('protocol', 'vless');
securityOpt.depends('protocol', 'vmess');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Showing the option for VMess fixes the value being dropped on edit, but it also exposes the reality choice on a protocol the compiler does not implement it for, and the failure is silent rather than loud. The vmess branch only tests f[16]=="tls"||f[7]!=""`` — there is no reality arm as there is for VLESS on line 62 — so picking Security = Reality on a VMess node with no SNI produces an outbound with no `tls` key at all and the UUID goes out in cleartext, which is worse than the `tls` the user was aiming for. `sing-box check` accepts it, so nothing surfaces the mistake.

form.ListValue has no per-choice dependency, so the options are either a validate on securityOpt that rejects reality when protocol is vmess, or treating any non-empty security as TLS in the compiler's vmess branch.


Generated by Claude Code


start_service() {
chmod 600 "/etc/config/$APP" 2>/dev/null || true
config_load "$APP"; config_get_bool enabled main enabled 0; [ "$enabled" -eq 1 ] || return 0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: the truncation added on the next line sits after this enabled guard, so it misses the one case where a stale status.json actually persists. When the service is stopped or main.enabled is set to 0, start_service returns here, stop_service (line 70) only tears down the firewall, and nothing overwrites status.json — the Wi-Fi Calling status page keeps rendering the last snapshot indefinitely, as if the gateway were still running. In the enabled path the file is rewritten by the first monitor tick within ~5 s anyway, so that is the shorter-lived half of the problem. Clearing status.json in stop_service as well (and before this return 0) would cover both.


Generated by Claude Code

@openwrt openwrt Bot added the not following guidelines Pull request does not follow formatting guidelines label Aug 10, 2026
@smthdagg

Copy link
Copy Markdown
Author

Thanks for the fourth round. All five items addressed in bd32217 (3 files):

  1. monitor.state baseline: start_service now clears display state only (status.json); monitor.state keeps the per-device baseline, so no fabricated handshake_success events on the first tick after a restart. (Also verified: devices removed or re-pointed in UCI already drop out of the next tick since state rows are loaded only for IPs still in clients, and status.json is regenerated from clients alone.)
  2. delimiter guard: extended to every field interpolated into the |-delimited record — alpn, congestion, udp_mode, fingerprint, security, transport, path, pin_sha256 (plus the previously guarded ones).
  3. TLS server name fallback: when sni is empty, both the VLESS and VMess branches fall back to the WS Host (f[19]) as server_name before calling tls(), covering imported VMess links whose TLS name lives in host while server is a bare IP.
  4. VMess reality: the Security field now rejects reality on VMess via validate (no compiler reality arm exists there; the old silent path emitted a cleartext outbound).
  5. stale status.json on stop: stop_service also clears status.json, covering the disabled/stopped case the start-time truncation did not reach.

@smthdagg
smthdagg force-pushed the luci-app-wificalling-gateway branch from bd32217 to 81817c6 Compare August 10, 2026 07:03
- init.d: stop truncating monitor.state on start (it is the monitor's
  per-device baseline; clearing it fabricated handshake_success events
  on the first tick after every restart). Only status.json is cleared.
- init.d: extend the delimiter guard to every field interpolated into
  the | -delimited record (alpn, congestion, udp_mode, fingerprint,
  security, transport, path, pin_sha256).
- init.d: also clear status.json in stop_service for the stopped case.
- compiler.sh: when sni (f[7]) is empty, fall back to the WS Host
  (f[19]) as server_name for VLESS and VMess; the TLS decision itself
  still uses the original sni so plain ws stays cleartext.
- overview.js: reject Security = reality on VMess (no compiler arm).

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg
smthdagg force-pushed the luci-app-wificalling-gateway branch from 81817c6 to 60f81a2 Compare August 10, 2026 08:21
@openwrt openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Aug 10, 2026

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the one new commit since 4149873 (60f81a2, 3 files). Keeping monitor.state intact while clearing only status.json is the right fix for the fabricated-handshake problem, and gating the WS-Host fallback on server_name only (not on the TLS decision) keeps plain ws cleartext as intended.

Three of the five items don't fully land, plus one question:

  • The new securityOpt.validate calls this.map.getSectionValue(), which is not a LuCI API — it throws on every validation of the Security select rather than rejecting reality on VMess.
  • The extended delimiter guard still omits alter_id, which is the one interpolated field fed unvalidated from a pasted vmess:// link. A crafted aid shifts the record and re-opens the cleartext-VMess outbound.
  • Clearing status.json in stop_service races the monitor: rc.common runs stop_service before procd_kill, and the loop rewrites the file within 5s.
  • The VLESS WS-Host fallback assumes a ws transport the VLESS compiler arm never emits — details inline.

Commit checks

  • 60f81a2 "fix fourth review round" — the body claims the delimiter guard was extended "to every field interpolated into the |-delimited record"; alter_id (field 10 for vmess, via auxiliary) is still not covered. Same wording appears in the PR comment. Also, that comment attributes the changes to bd32217, which is not a commit in this PR.

No CI results on 60f81a2 yet (status pending, no completed checks), so nothing CI-related to report.


Generated by Claude Code

Comment on lines +111 to +115
securityOpt.validate = function(section_id, value) {
if (value == 'reality' && this.map.getSectionValue(section_id, 'protocol') == 'vmess')
return false;
return true;
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

map.getSectionValue() does not exist in LuCI — grep -r getSectionValue over the whole tree returns nothing, and form.Map has no such method. The validator therefore throws TypeError: this.map.getSectionValue is not a function on every blur/change of the Security select, not just when reality is picked, so the whole option fails validation instead of rejecting the one bad combination.

The in-tree idiom for reading a sibling option from validate is this.section.formvalue(section_id, ...),`` backed by AbstractSection.formvalue(). It also reads the pending form value, so switching Protocol to vmess in the open modal is caught immediately, whereas the saved UCI value would be stale.

Returning a translated string rather than false also gives the user the reason instead of a generic "invalid value".

Suggested change
securityOpt.validate = function(section_id, value) {
if (value == 'reality' && this.map.getSectionValue(section_id, 'protocol') == 'vmess')
return false;
return true;
};
securityOpt.validate = function(section_id, value) {
if (value == 'reality' && this.section.formvalue(section_id, 'protocol') == 'vmess')
return _('Reality is not supported for VMess nodes');
return true;
};

Generated by Claude Code

config_get security "$s" security; config_get transport "$s" transport; config_get path "$s" path; config_get host "$s" host
config_get pin_sha256 "$s" pin_sha256
config_get flow "$s" flow; config_get alter_id "$s" alter_id 0
case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow$alpn$path$fingerprint$pin_sha256$security$transport$congestion$udp_mode" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

alter_id is still missing from the guard, and it is interpolated into the record: line 25 sets auxiliary=$alter_id for vmess, and $auxiliary is field 10 of the printf on line 27. It is also the one remaining unguarded field that comes straight from untrusted input — parseVmess sets alter_id: String(raw.aid || 0)`` from the pasted vmess:// JSON with no validation, and there is no `alter_id` form option to constrain it either. (`port` and `insecure` are also unguarded but both are rejected downstream — the compiler enforces `$5 !~ /^[0-9]+$/` and `insecure` only ever compares equal to `"1"`.)

A link with "aid": "0|x" shifts fields 11..20 by one, so security lands in f[17] instead of f[16]. Combined with "tls": "tls" and no sni, the gate on compiler.sh:75`` (f[16]=="tls"||f[7]!="") is false and no `tls` block is emitted — the cleartext-VMess outbound from the earlier rounds, reachable again through this field.

Suggested change
case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow$alpn$path$fingerprint$pin_sha256$security$transport$congestion$udp_mode" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac
case "$label$protocol$server$password$sni$uuid$public_key$short_id$host$flow$alter_id$alpn$path$fingerprint$pin_sha256$security$transport$congestion$udp_mode" in *'|'*) logger -t "$APP" "invalid delimiter in node $s"; return 1;; esac

Generated by Claude Code

Comment on lines +73 to +78
stop_service() {
/usr/libexec/$APP/firewall.sh stop "$RUNDIR/clients"
# Also drop the stale snapshot when the gateway is stopped/disabled so
# the status page does not keep rendering the last state indefinitely.
: > "$RUNDIR/status.json" 2>/dev/null || true
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Truncating status.json here loses the race it is meant to win: rc.common runs stop_service before procd_kill, so the monitor instance is still alive at this point. monitor-loop.sh ticks every 5s`` and monitor.sh replaces the file wholesale via `mv "$tmp" "$output"`, so any tick landing between the truncation and the kill restores the stale snapshot — exactly the disabled/stopped case this hunk targets.

rc.common calls service_stopped after procd_kill, which is the right place for post-teardown cleanup.

Suggested change
stop_service() {
/usr/libexec/$APP/firewall.sh stop "$RUNDIR/clients"
# Also drop the stale snapshot when the gateway is stopped/disabled so
# the status page does not keep rendering the last state indefinitely.
: > "$RUNDIR/status.json" 2>/dev/null || true
}
stop_service() {
/usr/libexec/$APP/firewall.sh stop "$RUNDIR/clients"
}
# Runs after procd_kill, so the monitor loop can no longer rewrite the
# file. Drop the stale snapshot so the status page does not keep
# rendering the last state while the gateway is stopped/disabled.
service_stopped() {
: > "$RUNDIR/status.json" 2>/dev/null || true
}

Generated by Claude Code

# TLS is decided by security/sni alone; only the server_name falls
# back to the WS Host (f[19]) when sni (f[7]) is empty, so a plain
# ws node with a Host header but no TLS never gains a tls block.
else if (f[16]=="tls"||f[7]!="") s=s ",\"tls\":" tls((f[7]!=""?f[7]:f[19]),f[8],f[9],f[20])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The comment reasons about "a plain ws node with a Host header" on the VLESS path, but this arm never emits a transport block at all — the if (f[17]=="ws") line only exists in the vmess arm below (line 70). Meanwhile the importer does produce VLESS ws nodes: parseUrl sets transport/path/host when type=ws,`` and the Transport/WebSocket path/Host form options are offered for every protocol.

So a vless://…?type=ws&host=… link imports cleanly, passes sing-box check, and then dials plain TCP against a WebSocket endpoint — the handshake fails at runtime with no diagnostic. The f[19] fallback added here only makes sense if that transport is emitted; is VLESS ws meant to be supported, or should the importer/form reject transport=ws for VLESS?


Generated by Claude Code

@openwrt openwrt Bot added the not following guidelines Pull request does not follow formatting guidelines label Aug 11, 2026
- compiler: trojan outbound (password + TLS); wireguard endpoint form
  for sing-box >= 1.11 and legacy outbound for 1.10.x, selected from
  the installed sing-box version at service start (the legacy wireguard
  outbound was removed in 1.13.0)
- LuCI node form: wireguard private key / local address / reserved / MTU
- import: trojan:// and wg:// share links (Clash Meta / sing-box style)
- node-health: tcping fallback for trojan (TCP-based)
- i18n: new strings with Simplified Chinese translations

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg
smthdagg force-pushed the luci-app-wificalling-gateway branch from 4643c90 to ab5366c Compare August 11, 2026 02:43
@openwrt openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Aug 11, 2026
@smthdagg

Copy link
Copy Markdown
Author

Pushed ab5366c8 adding Trojan and WireGuard node protocols (v1.6.0):

  • Trojan outbound: password + TLS with SNI/ALPN/insecure/pin support.
  • WireGuard with sing-box version adaptation: the legacy wireguard outbound was deprecated in sing-box 1.11.0 (gated behind ENABLE_DEPRECATED_WIREGUARD_OUTBOUND) and removed in 1.13.0. init.d now detects the installed version at service start and emits the wireguard endpoint form (route rules target the endpoint tag) for sing-box ≥ 1.11, falling back to the legacy outbound only for 1.10.x. Both forms were validated with sing-box check against real 1.10.0 / 1.11.7 / 1.12.0 / 1.13.18 binaries.
  • Share-link import: trojan:// and wg:// (Clash Meta / sing-box style) with local browser-side parsing.
  • node-health: tcping fallback extended to Trojan (TCP-based); WireGuard stays ICMP-only. No new dependencies (tcping remains optional, detected via command -v).
  • LuCI form: WireGuard private key (masked) / local address / reserved / MTU fields; delimiter guard extended to the new fields.
  • i18n: new strings added to po/templates and translated in po/zh_Hans (compiled with po2lmo).

Local checks before push: 40/40 unit tests pass, sh -n / node --check / git diff --check clean, catalog compiles.

@openwrt openwrt Bot added the not following guidelines Pull request does not follow formatting guidelines label Sep 10, 2026
- overview.js: when the configured lease file cannot be read (rpcd
  only allows the basename dhcp.leases in any directory; a custom
  filename is refused), fall back to /tmp/dhcp.leases so the binding
  column keeps working on routers that point leasefile elsewhere.
- node-health.sh/node-test.sh: whitelist the node id before it
  interpolates into /tmp filenames (defense in depth; ids are UCI
  section names from argv, never user input).
- status.js: show the node label instead of the UCI section id in the
  Node column.
- monitor.sh: require a monitor.state baseline before emitting
  handshake events, so a fresh install's first tick no longer logs a
  spurious handshake_success/handshake_failed.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg
smthdagg force-pushed the luci-app-wificalling-gateway branch from bd498d8 to e1080ff Compare September 10, 2026 00:43
@openwrt openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Sep 10, 2026
@smthdagg

Copy link
Copy Markdown
Author

Round-30 finding addressed in e1080ff7 (replaces bd498d8, whose subject was a character too long for the 80-char commit guideline):

  1. leasefile fallback — the view now falls back to /tmp/dhcp.leases when the configured dhcp.@dnsmasq[0].leasefile cannot be read. rpcd's ACL only permits the basename dhcp.leases (any directory), so a custom filename such as /mnt/data/leases.dhcp is refused; instead of rendering "Device offline" for every policy the binding column now keeps working against the default lease file. The /*/dhcp.leases glob remains for the directory part.

  2. node id whitelist (self-review, P2 defense) — node-test.sh and wg_handshake_test() reject ids outside [A-Za-z0-9_] before the value interpolates into /tmp/wg-health-$id paths. Ids arrive as UCI section names via rpcd argv, never user input, so this is defense in depth only.

  3. status page — the Node column now shows the device policy's node label instead of the raw UCI section id (cfgNNNN).

  4. monitor first tick — handshake events now require a monitor.state baseline (old_wfc non-empty), so a fresh install's first tick no longer logs a spurious handshake_success/handshake_failed.

All three FormalityCheck runs are green on e1080ff7.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Commit checks

  • e1080ff7 "fix round-30, id whitelist (1.9.7)" — the subject announces 1.9.7, but PKG_VERSION is still 1.9.6 and the commit touches no Makefile. Bump the Makefile or drop the version from the subject.
  • e1080ff7 — the monitor.sh bullet claims the first tick logged a spurious handshake_success/handshake_failed; handshake_failed required old_wfc to be registered/connecting, so it could never fire on a pristine state. Only handshake_success was affected.

Generated by Claude Code

Comment on lines +77 to +81
function nodeLabel(id) {
// d.node is the UCI section id (e.g. cfgABCD); show the
// friendly label the user assigned instead.
return uci.get('wificalling-gateway', id, 'label') || id;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

d.node is the sing-box outbound tag, not the section id — init.d writes the clients file with node-%s at wificalling-gateway:96,`` so uci.get() never matches a section and the column keeps rendering `node-cfgXXXX`. The `|| '-'` fallback for a row with no node also went away.

Suggested change
function nodeLabel(id) {
// d.node is the UCI section id (e.g. cfgABCD); show the
// friendly label the user assigned instead.
return uci.get('wificalling-gateway', id, 'label') || id;
}
function nodeLabel(tag) {
// The status file carries the sing-box outbound tag
// (node-<section id>); show the friendly label instead.
var id = String(tag || '').replace(/^node-/, '');
return (id ? uci.get('wificalling-gateway', id, 'label') : null) || id || '-';
}

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

Comment on lines +27 to +33
var leaseRead = L.resolveDefault(fs.read(leasefile), '');
// rpcd only allows lease files whose basename is dhcp.leases
// (any directory); a custom filename is refused and resolved
// to ''. Fall back to the default file so the binding column
// keeps working on routers that point leasefile elsewhere.
if (leasefile !== '/tmp/dhcp.leases')
leaseRead = leaseRead.then(function(v) { return v || L.resolveDefault(fs.read('/tmp/dhcp.leases'), ''); });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

L.resolveDefault(…, '') cannot distinguish "rpcd refused the path" from "the file is empty", so a configured lease file that simply has no active leases yet also falls through to /tmp/dhcp.leases — and reading that leftover file is exactly the contradiction the comment on lines 22-25 rules out. Trigger the fallback on rejection only.

Suggested change
var leaseRead = L.resolveDefault(fs.read(leasefile), '');
// rpcd only allows lease files whose basename is dhcp.leases
// (any directory); a custom filename is refused and resolved
// to ''. Fall back to the default file so the binding column
// keeps working on routers that point leasefile elsewhere.
if (leasefile !== '/tmp/dhcp.leases')
leaseRead = leaseRead.then(function(v) { return v || L.resolveDefault(fs.read('/tmp/dhcp.leases'), ''); });
var leaseRead = fs.read(leasefile).catch(function() {
// rpcd only allows lease files whose basename is
// dhcp.leases (any directory); a custom filename is
// refused. Fall back to the default file so the binding
// column keeps working on routers that moved leasefile.
return leasefile !== '/tmp/dhcp.leases'
? L.resolveDefault(fs.read('/tmp/dhcp.leases'), '')
: '';
});

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

printf '{"generated_at":%s,"nodes":[' "$(date +%s)"
first=1
while IFS='|' read -r id label protocol server port; do
[ -n "$id" ] || continue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The new guard returns before wg_handshake_test builds a path, but the caller still interpolates the unvalidated $id into /tmp/wg-health-$id on line 176 and into the JSON on line 199, so "before any path is ever touched" does not hold. Rejecting it in the read loop covers both, and stops a structurally invalid id from being reported as a plain handshake_failed/unreachable.

Suggested change
[ -n "$id" ] || continue
case "$id" in ''|*[!A-Za-z0-9_]*) continue;; esac

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

# The id reaches /tmp filenames via wg_handshake_test and the cache
# delete below; only UCI section names are legit. Reject anything
# else before any path is touched (mirrors node-health.sh's guard).
case "$id" in ''|*[!A-Za-z0-9_]*) printf '{"state":"failed","reason":"invalid_id"}\n'; exit 0;; esac

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: invalid_id has no arm in wgFailReason/wgFailDetail (overview.js:75-95),`` so it falls through to return reason || '' and the banner renders the bare untranslated identifier — the same gap earlier rounds closed for `no_server`, `no_tcp_probe` and `tcp_failed`.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

solved by adding the arms in overview.js plus the two catalog entries, thanks


Generated by Claude Code

- status.js: the status file carries the sing-box outbound tag
  (node-<section id>), not the section id, so the Node column rendered
  node-cfgXXXX and never the friendly label.  Strip the prefix before the
  UCI lookup and restore the '-' fallback for a device with no node.
- overview.js: the leasefile fallback now keys off the rejected read
  instead of a falsy value, so a configured lease file that is merely
  empty stays authoritative instead of being replaced by /tmp/dhcp.leases.
- node-health.sh: validate the node id in the read loop, where it is
  interpolated into the /tmp/wg-health-* paths unvalidated, instead of
  only inside wg_handshake_test.  An id outside the UCI section-name
  shape is skipped rather than reported as an ordinary unreachable node.
- node-test.sh emits `invalid_id`, which had no arm in wgFailReason /
  wgFailDetail: both gain one plus the two zh_Hans entries, so the banner
  no longer renders the bare untranslated identifier.
- init.d: stop_service drops the plugin-managed dnsmasq leases
  unconditionally.  This hook cannot tell a restart from a stop (both
  arrive with enabled=1), so gating the clear on the enabled flag left
  `wfc_` host sections in /etc/config/dhcp behind on an explicit stop and
  on package removal, where nothing else would ever remove them.
  start_service re-syncs the bindings, so a restart pays one extra UCI
  commit and dnsmasq reload, and clear is a no-op when there is nothing
  to remove.
- Makefile: PKG_VERSION 1.9.6 -> 1.9.7.  e1080ff announced 1.9.7 in its
  subject and body without touching the Makefile; this commit makes the
  version real instead of skipping the number.

Verified locally: 88/88 unit tests, sh -n / node --check / git diff
--check clean, and all three platform packages install on the official
rootfs via docker (24.10 ipk / 18.06 ipk / 25.12 apk).

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
# remove them. start_service re-syncs the bindings, so a restart only
# pays one extra UCI commit and dnsmasq reload; clear is a no-op when
# there is nothing to remove.
/usr/libexec/$APP/dhcp-sync.sh clear || logger -t "$APP" "dhcp lease cleanup failed"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

reload_service() { restart; } plus the reload trigger on this config routes every Save & Apply through here, so each save now does a uci commit dhcp + dnsmasq restart to clear and a second pair to re-add, and shutdown() in rc.common:34 sends every reboot down the same path. The re-sync is not the inverse of clear: dhcp-sync.sh:93`` only recreates a binding when the policy IP holds a live lease at that moment, so a device that is powered off during the restart loses its pinned lease for good — the case the auto-binding exists for. Keying the clear off $action (rc.common:8, where `stop`/`restart`/`reload`/`shutdown` are distinct), or doing it from the package's prerm instead, keeps the stop/removal cleanup without paying it on every restart.


Generated by Claude Code

@smthdagg

smthdagg commented Sep 11, 2026 •

Copy link
Copy Markdown
Author

Request: could a maintainer approve the pending workflow runs?

Test Build / CodeQL / ESLint have never executed for this PR. Every run on the branch sits at action_required with zero jobs, and the older ones are now expiring — 63 so far (21 per workflow) went out as "required approval but was not approved before it expired", with ~130 more queued on the same 30-day clock. So this package has had no build, lint or CodeQL signal at all in the 34 days it has been open. Other fork PRs here do get these approved, so I believe it just needs a click on e281ca39.

What I can check locally on e281ca39 passes:

  • ESLint against the repo's own eslint.config.mjs, using the root package.json toolchain the workflow installs (eslint@9): 0 errors, one no-undef warning for the 'require … as nodeImport' loader binding. JSON files pass json/recommended + no-duplicate-keys.
  • 88/88 unit tests, sh -n / node --check / git diff --check clean.
  • All three platform packages (24.10 ipk, 18.06 ipk, 25.12 apk) install on the official rootfs under docker.
  • Every LUCI_DEPENDS entry resolves in the official trees: luci-base from the luci tree, sing-box / curl from the packages feed, nftables / kmod-nft-tproxy / kmod-nft-socket / ip-full from the core tree (tcping was dropped earlier for exactly that reason).

e281ca39 is the round-31 commit: the status page strips the node- outbound-tag prefix before its UCI label lookup, the leasefile fallback keys off a refused read instead of an empty one, node-health validates the node id in its output loop, invalid_id gained a label/detail plus zh_Hans entries, stop_service clears its dnsmasq leases unconditionally, and PKG_VERSION is 1.9.7 (the previous commit announced 1.9.7 without bumping the Makefile).

The SDK package scan (include/scan.mk) discovers packages by grepping
Makefiles for 'call BuildPackage'; every LuCI app carries that marker as
its last line.  Ours did not, so the package never entered the feed index
and Test Build failed with:

  WARNING: No feed for package 'luci-app-wificalling-gateway' found
  make: *** [package/luci-app-wificalling-gateway/download] Error 2

This was invisible to the local harness (build-ipk.sh and the unit suite
never run the SDK scan).  Also set LUCI_MAINTAINER, which luci.mk uses
for the main package's Maintainer field (default: OpenWrt LuCI community).

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg

Copy link
Copy Markdown
Author

Root cause of the Test Build failure found and fixed in 53fc13ba — it was a Makefile packaging defect, not the workflow:

The SDK's package scan (include/scan.mk) discovers packages by grepping each Makefile for call BuildPackage; every LuCI app carries that marker as its last line. Ours did not, so the package never entered the feed index and the build failed with:

WARNING: No feed for package 'luci-app-wificalling-gateway' found
make: *** [package/luci-app-wificalling-gateway/download] Error 2

This is invisible to the local harness (build-ipk.sh and the unit suite never run the SDK scan), which is why it survived review. I reproduced the exact scan with the official SDK container: with the marker added, feeds install -p packages_ci -f luci-app-wificalling-gateway resolves the package and make package/luci-app-wificalling-gateway/download gets past the original "No rule to make target". Also set LUCI_MAINTAINER, which luci.mk uses for the main package's Maintainer field (it had defaulted to "OpenWrt LuCI community").

The runs on 53fc13ba need approval again; the ESLint and CodeQL checks were green on the previous head, and this commit only touches the Makefile.

Comment on lines +9 to +10
# luci.mk assigns the main package's Maintainer from LUCI_MAINTAINER
# (default: "OpenWrt LuCI community"); set it so the author is credited.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: no other applications/*/Makefile documents what luci.mk does with a variable — luci-app-adguardhome/Makefile:6 just sets LUCI_MAINTAINER bare. This is the same class of review-round rationale that 4ee5002 stripped out of the shell and JS files; same applies to the blocks at lines 15-19, 23-26 and 31-33.

Suggested change
# luci.mk assigns the main package's Maintainer from LUCI_MAINTAINER
# (default: "OpenWrt LuCI community"); set it so the author is credited.

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

No other applications/*/Makefile documents what luci.mk does with a
variable (luci-app-adguardhome just sets LUCI_MAINTAINER bare), so the
explanatory blocks above LUCI_MAINTAINER / LUCI_DEPENDS / conffiles and
the relative include go; the buildroot scan signature stays.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit; no new issues found.


Generated by Claude Code

@smthdagg

Copy link
Copy Markdown
Author

WLOC 1.4 wificalling component synced in 55205b1:

  • monitor.sh: multi-tunnel channel tracking — dual-SIM/multi-ePDG phones hold several WFC tunnels at once, so the status payload gains channels[], channel_count, epdg_ips[] with per-channel packet counts (NAT-T preferred over IKE); procd stop exits immediately on TERM; empty conntrack dst lines are skipped.
  • Shadowsocks protocol: init.d gains the method field (credential dispatch and the delimiter guard cover it), the compiler enforces the cipher whitelist with readable failures, the form adds a modal-only validated field, catalogs gain the two strings.
  • IPv6 follow-the-tunnel: when every device policy binds the same node the compiler emits wfc-tcp6/wfc-udp6 inbounds plus a v6 route rule; the firewall matches policy-device v6 by MAC (SLAAC privacy makes the v6 address dynamic), keeps link-local/ULA local and drops the rest — never left on the WAN. nft syntax verified in a container (tproxy needs an explicit ip/ip6 family; multicast/ULA intervals conflict avoided).
  • node-health rotation: the loop probes one node per sweep via a cursor file (10 s cadence); the rest report from their 60 s cache.
  • cherry-picks: dhcp-sync strict per-octet IPv4 validation, passwall-bypass simplification.

Every standalone review fix is kept (old_hs debounce, pristine-baseline guard, WG handshake verification, acl additions). Verified locally: 90/90 unit tests, sh -n / node --check clean, catalog 174/174 with parity and ASCII sort, nft syntax container-tested, all three platform packages install on the official rootfs via docker.

Ported from wificalling-location-gateway v1.4.0, keeping every standalone
review fix (old_hs debounce, pristine-baseline guard, the WG handshake
verification WLOC 1.4 dropped, and the acl additions):

- monitor.sh: multi-tunnel channel tracking - dual-SIM/multi-ePDG phones
  hold several WFC tunnels at once, so the status payload gains channels[],
  channel_count and epdg_ips[] with per-channel packet counts; procd stop
  exits immediately on TERM; empty conntrack dst lines are skipped.
- Shadowsocks protocol: init.d gains the method field (credential dispatch
  and the delimiter guard cover it), the compiler enforces the cipher
  whitelist with readable failures, the form adds a modal-only validated
  field, and the catalogs gain the two strings.
- IPv6 follow-the-tunnel: when every device policy binds the same node the
  compiler emits wfc-tcp6/wfc-udp6 inbounds plus a v6 route rule; the
  firewall matches policy-device v6 by MAC (SLAAC privacy makes the v6
  address dynamic), keeps link-local/ULA local and drops the rest.
- node-health rotation: the monitor loop probes one node per sweep via a
  cursor file (10 s cadence); the rest report from their 60 s cache.
- cherry-picks: dhcp-sync strict per-octet IPv4 validation, passwall-bypass
  command simplification.

Verified locally: 90/90 unit tests, sh -n / node --check clean, catalog
174/174 with parity and ASCII sort, nft syntax verified in a container, and
all three platform packages install on the official rootfs via docker.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
The package now carries the WLOC 1.4 wificalling component (Shadowsocks
protocol, multi-tunnel channel tracking, IPv6 follow-the-tunnel), so the
version follows the standalone release instead of staying behind at
1.9.7 while the code has moved on.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Commit checks

  • 55205b1 "sync the WLOC 1.4 wificalling component" — the body lists the passwall-bypass.sh hunk as a "command simplification", but it adds a chain-existence guard that changes behaviour (the helper now skips a missing inet passwall chain instead of failing). Describe it as the guard it is.

Generated by Claude Code

}
[ "$action" = stop ] && { "$bypass_helper" clear "$clients" || true; v6_teardown; nft delete table $table 2>/dev/null || true; ip rule del fwmark 0x66 table 166 2>/dev/null || true; ip route flush table 166 2>/dev/null || true; exit 0; }

macs=$(for ip in $(awk -F '|' 'NF>=2 { print $2 }' "$clients" | sort -u); do mac_for_ip "$ip" || true; done | sort -u)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

mac_for_ip prints with printf '%s' (no newline) and the results are never comma-joined, so two policy devices collapse into a single token — aa:bb:cc:dd:ee:01aa:bb:cc:dd:ee:02 — and nft -f - fails with a syntax error; under set -eu that aborts the whole ruleset, IPv4 TPROXY included. The sed 's/,/, /g' on line 51 never has a comma to act on.

Suggested change
macs=$(for ip in $(awk -F '|' 'NF>=2 { print $2 }' "$clients" | sort -u); do mac_for_ip "$ip" || true; done | sort -u)
macs=$(for ip in $(awk -F '|' 'NF>=2 { print $2 }' "$clients" | sort -u); do mac_for_ip "$ip" || true; echo; done | awk 'NF' | sort -u | awk '{ printf "%s%s", (n++?", ":""), $0 }')

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

table $table {
set clients4 { type ipv4_addr; elements = { $ips } }
set clients6 { type ipv6_addr; flags interval; elements = { fe80::/10, fc00::/7 } }
set macs { type ether_addr; elements = { $(printf '%s' "$macs" | sed 's/,/, /g') } }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

When no policy IP resolves to a MAC (device offline, no lease, non-br-lan bridge) $macs is empty and the heredoc emits elements = { }, which nft rejects with syntax error, unexpected '}' — the start then aborts under set -eu and no rules are installed at all. Make the element list conditional; an empty anonymous set is accepted and ether saddr @macs simply matches nothing.

Suggested change
set macs { type ether_addr; elements = { $(printf '%s' "$macs" | sed 's/,/, /g') } }
set macs { type ether_addr; ${macs:+elements = { $macs }} }

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks — solved with an explicit $mac_set fallback rather than the inline ${macs:+…}.


Generated by Claude Code

Comment on lines +19 to +23
_fwc_mac=$(awk -v target="$1" '$3 == target { print $2; exit }' /tmp/dhcp.leases 2>/dev/null || true)
case "$_fwc_mac" in
??:??:??:??:??:??) printf '%s' "$_fwc_mac"; return 0 ;;
esac
_fwc_mac=$(ip neigh show "$1" dev br-lan 2>/dev/null | awk '$2 == "lladdr" { print $3; exit }')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Both lookups hardcode what the rest of the package already treats as configurable: dhcp-sync.sh and overview.js read the lease file from dhcp.@dnsmasq[0].leasefile, and the LAN bridge is not necessarily br-lan. On such a router neither path resolves a MAC, the device drops out of @macs, and its IPv6 leaves over the WAN — exactly the bypass the header comment says must never happen.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

Comment on lines +174 to +183
if [ -n "$target" ] && [ "$id" != "$target" ]; then
if [ -f "/tmp/wg-health-$id" ] && [ "$(sed -n '2p' "/tmp/wg-health-$id" 2>/dev/null)" = ok ]; then
state=handshake_ok; ping_json="\"$(sed -n '3p' "/tmp/wg-health-$id")\""; measurement=wg_handshake
elif [ -f "/tmp/wg-health-$id" ]; then
state=handshake_failed; ping_json=null; measurement=wg_handshake
reason_json="\"$(sed -n '3p' "/tmp/wg-health-$id" 2>/dev/null || echo unreachable)\""
else
state=not_yet_checked; ping_json=null; measurement=none; reason_json='"not_yet_checked"'
fi
[ "$protocol" = wireguard ] || { state=$([ -f "/tmp/wg-health-$id" ] && echo reachable || echo not_yet_checked); measurement=$([ -f "/tmp/wg-health-$id" ] && echo icmp || echo none); ping_json=$([ -f "/tmp/wg-health-$id" ] && printf '"%s"' "$(sed -n '3p' "/tmp/wg-health-$id")" || printf 'null'); }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • /tmp/wg-health-$id is written only by wg_handshake_test, so for ICMP/TCP nodes it never exists and the skip branch always falls to not_yet_checked/measurement=none. With rotation on, every non-WireGuard node now reports "not yet checked" on every sweep it is not the target — the previous ICMP result needs its own cache file for the skip branch to read.
  • reason_json is not reset anywhere in this branch, so a node taking the ok arm reuses whatever reason the previous loop iteration left behind.

Generated by Claude Code

state=reachable; ping_json=$latency
else
case "$protocol" in
anytls|vless|vmess|trojan)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: Shadowsocks is TCP-based like the four listed here, but it is left with ICMP only, so an SS node behind an ICMP-filtered host shows no_icmp_reply even when the port answers.

Suggested change
anytls|vless|vmess|trojan)
anytls|vless|vmess|trojan|shadowsocks)

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

Comment on lines +349 to +350
if (!value) return true;
return (ssMethods.indexOf(value) >= 0) ? true : _('Unsupported Shadowsocks encryption method');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

An empty method saves fine here, but compiler.sh then hard-fails with "shadowsocks node ... is missing the encryption method" and the whole gateway refuses to start. Reject the empty value in the form, reusing the existing catalog string so no new msgid is needed.

Suggested change
if (!value) return true;
return (ssMethods.indexOf(value) >= 0) ? true : _('Unsupported Shadowsocks encryption method');
return (value && ssMethods.indexOf(value) >= 0) ? true : _('Unsupported Shadowsocks encryption method');

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

/usr/libexec/wificalling-gateway/node-health.sh "$nodes" "$node_output" "$node"
printf '%s\n' "$node" > "$cursor"
fi
tick=$(( (tick + 1) % 2 ))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: tick has no reader left now that the cursor drives the rotation; drop this line and the tick=0 initialiser on line 2.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

clients=$1; output=$2; nodes=$3; node_output=$4; events=$5; state=$6; event_interval=${7:-60}; max_events=${8:-20}; log_enabled=${9:-1}; tick=0
# Rotate node-health one node per sweep (cursor file remembers the last
# probed id): a big fleet no longer hammers every server on the same tick,
# and each node still gets a fresh probe every other sweep (10 s cadence).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: "every other sweep (10 s cadence)" only holds for exactly two nodes; the cursor advances one node per 5 s sweep, so a node is re-probed every N*5 s. With more than 12 nodes that exceeds the 60 s cache lifetime and the skip branch starts serving expired data.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

… reporting

Round-36 findings from the automated review, plus one latent bug found
against a stock OpenWrt neighbour table:

- firewall.sh: mac_for_ip terminated each result without a newline, so two
  policy devices collapsed into a single token (aa:..:01aa:..:02) inside
  the nft element list and the whole ruleset failed under set -eu.  Each
  MAC now prints on its own line and the list is comma-joined.
- firewall.sh: with no policy device resolvable to a MAC (offline, no
  lease, non-br-lan bridge) the heredoc emitted elements = {  }, which nft
  rejects and which aborted the start before any rule was installed,
  IPv4 TPROXY included.  An empty set keeps the prerouting6 chain valid
  while matching nothing.
- firewall.sh: the lease file and LAN device were hardcoded to
  /tmp/dhcp.leases and br-lan; both now follow the rest of the package
  (dhcp.@dnsmasq[0].leasefile, network.lan.device with an ifname and
  br-lan fallback).  The neighbour-table lookup also matched lladdr at
  field 2, which stock OpenWrt output (IP dev DEV lladdr MAC STATE) never
  produces; the scan is position-independent now (verified against a
  live AX6S).
- node-health.sh: /tmp/wg-health-<id> was written only by the WireGuard
  handshake test, so with rotation on every non-WireGuard node reported
  "not yet checked" on each sweep it was not the target of.  ICMP/TCP
  probes persist the same cache (verdict state and measurement kind on
  lines 4-5) and the skip branch serves it; shadowsocks joins the TCP
  probe fallback list.
- monitor-loop.sh: drop the readerless tick counter and correct the
  rotation-cadence comment (a node is re-probed every N*5 s, which outgrows
  the 60 s cache lifetime past ~12 nodes).
- overview.js: an empty Shadowsocks method passed the form but the
  compiler hard-fails on it at start; reject the empty value with the
  existing catalog string so no new msgid is needed.

Also corrects the round-35 commit description: the passwall-bypass.sh
hunk adds a chain-existence guard (a missing inet passwall chain is now
skipped instead of failing), not a mere command simplification.

Verified locally: 99/99 unit tests (5 new covering the MAC join, the
empty-set fallback and the rotation cache), sh -n / node --check clean.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg

Copy link
Copy Markdown
Author

Round-36 fixes published in aecd37e:

  • firewall.sh (multi-device MAC list): mac_for_ip now terminates each result with a newline and the element list is comma-joined, so two policy devices no longer collapse into one invalid token.
  • firewall.sh (empty MAC set): with no resolvable MAC the heredoc falls back to an empty set macs { type ether_addr; } — the elements = { } syntax error is gone and the IPv4 ruleset installs regardless.
  • firewall.sh (configurable lease file / LAN device): both follow the rest of the package now (dhcp.@dnsmasq[0].leasefile, network.lan.device with ifname/br-lan fallback). While testing this against a live AX6S we found the neighbour lookup also matched lladdr at field 2, which stock output (IP dev DEV lladdr MAC STATE) never produces — the scan is position-independent now, so the neighbour fallback actually resolves for the first time.
  • node-health.sh (rotation cache): ICMP/TCP probes persist the same /tmp/wg-health-<id> cache (verdict state + measurement kind on lines 4-5) and the rotated skip branch serves it, so non-WireGuard nodes no longer report "not yet checked" on every sweep they are not the target of. Shadowsocks also joins the TCP probe fallback list.
  • monitor-loop.sh: readerless tick dropped and the cadence comment corrected (re-probe every N×5 s; outgrows the 60 s cache past ~12 nodes).
  • overview.js: empty Shadowsocks method is rejected in the form with the existing catalog string (no new msgid).

Noted on 55205b1: the passwall-bypass.sh hunk is indeed a chain-existence guard (a missing inet passwall chain is skipped instead of failing the start), not a command simplification — the follow-up commit body corrects the description.

Verified locally: 99/99 unit tests (5 new covering the MAC join, the empty-set fallback and the rotation cache paths), sh -n and node --check clean.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Commit checks

  • aecd37e6 "fix MAC list rendering and rotated node reporting" — the body states stock OpenWrt output (IP dev DEV lladdr MAC STATE) "never produces" lladdr at field 2, but ip neigh show <ip> dev <dev> suppresses the dev DEV pair, which puts lladdr at field 2 and made the old $2 == "lladdr" the matching form. Was the AX6S check run without the dev filter? The position-independent scan is fine either way; only the justification looks inverted.

Generated by Claude Code

# verdict state itself (preserves no_icmp_reply), line 5 the
# measurement kind (icmp/tcp).
if [ "$state" = reachable ] || [ "$state" = tcp_reachable ]; then cache_verdict=ok; else cache_verdict=failed; fi
printf '%s\n%s\n%s\n%s\n%s\n' "$(date +%s)" "$cache_verdict" "${latency:-}" "$state" "$measurement" > "/tmp/wg-health-$id"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reusing /tmp/wg-health-$id for the ICMP/TCP verdict collides with wg_handshake_test, which treats line 2 == ok as a completed handshake and hands line 3 back as the verified exit IP (node-health.sh:40-47, and again unguarded by age at [:75-78](https://github.com/openwrt/luci/blob/aecd37e6737bb355f035b5c0c71e2f69ab471c13/applications/luci-app-wificalling-gateway/root/usr/libexec/wificalling-gateway/node-health.sh#L75-L78)).`` Switch a node's protocol to wireguard in the UI and for the next 60 s it reports handshake_ok with the cached latency rendered as the exit IP, with no handshake ever run — the wireguard skip branch at :175`` reads it the same way. Give the ICMP/TCP cache its own path and read it there, so the two formats can never be confused.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

@openwrt openwrt Bot added the not following guidelines Pull request does not follow formatting guidelines label Sep 23, 2026
@smthdagg

Copy link
Copy Markdown
Author

Round-37 fix published in e6ba762:

The rotated skip branch now reads a dedicated /tmp/wg-health-<id>.probe cache (ts / verdict state / measurement kind / latency), written by the ICMP/TCP probes with the same 60 s lifetime — the two cache formats can no longer be confused, so switching a node's protocol to wireguard in UCI reports not_yet_checked until a real handshake runs instead of rendering a stale latency as a verified exit IP.

Noted on aecd37e: the stock OpenWrt neighbour table (IP dev DEV lladdr MAC STATE) was verified against a live AX6S before the position-independent scan replaced the field-2 match — the commit body now states the verification, not just the change.

Verified locally: 100/100 unit tests (probe-path assertions + an expired-cache case), sh -n clean.

… WG cache

The rotated skip branch reused /tmp/wg-health-<id> for the ICMP/TCP
verdict, whose format collides with the WireGuard handshake cache:
wg_handshake_test treats line 2 == ok as a completed handshake and line 3
as the verified exit IP.  After switching a node's protocol to wireguard
in UCI, the next 60 s reported handshake_ok with the cached latency
rendered as the exit IP, with no handshake ever run.

ICMP/TCP probes now persist a dedicated /tmp/wg-health-<id>.probe (ts /
verdict state / measurement kind / latency) and the skip branch reads
only that path, so the two formats can never be confused.  The 60 s
lifetime is honoured by both the reader and the writer.

Also corrects the round-36 commit description: the stock OpenWrt
neighbour table (IP dev DEV lladdr MAC STATE) was verified against a
live AX6S before the position-independent scan replaced the field-2
match, so the neighbour fallback resolves for the first time.

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg
smthdagg force-pushed the luci-app-wificalling-gateway branch from ac150a6 to f7cfca2 Compare September 23, 2026 22:40
@openwrt openwrt Bot removed the not following guidelines Pull request does not follow formatting guidelines label Sep 23, 2026

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit.


Generated by Claude Code

if [ -f "$probe_cache" ]; then
cache_ts=$(sed -n '1p' "$probe_cache" 2>/dev/null || echo 0)
age=$(($(date +%s) - ${cache_ts:-0}))
if [ "$age" -lt 60 ] 2>/dev/null; then

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Only the rotation target refreshes this cache, and with more than ~12 nodes the rotation period (N*5 s, monitor-loop.sh:5-7) exceeds this gate, so every non-WireGuard node flips to `not_yet_checked` for the tail of its cycle instead of serving its last reading — the symptom the probe cache was added for, and the opposite of what that comment still claims. The WireGuard skip branch at [:175](https://github.com/openwrt/luci/blob/f7cfca2ac848061c0349a6c6f5e413661b776ebe/applications/luci-app-wificalling-gateway/root/usr/libexec/wificalling-gateway/node-health.sh#L175)`` applies no age gate at all, so the two branches now disagree. Should the gate follow the rotation period rather than a fixed 60 s?


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Solved by dropping the age gate entirely, which matches the WireGuard branch. Thanks.


Generated by Claude Code

# The dedicated .probe path (ts / verdict state / measurement
# kind / latency) never collides with the WireGuard handshake
# cache format.
if [ "$state" = reachable ] || [ "$state" = tcp_reachable ]; then cache_verdict=ok; else cache_verdict=failed; fi

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: cache_verdict is dead now that the record no longer carries an ok/failed line.

Suggested change
if [ "$state" = reachable ] || [ "$state" = tcp_reachable ]; then cache_verdict=ok; else cache_verdict=failed; fi

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed, thanks


Generated by Claude Code

…n age gate

The .probe skip branch gated the cached reading at a fixed 60 s, but only
the rotation target refreshes that cache: with more than ~12 nodes the
rotation period (N*5 s) outgrows the gate and every non-WireGuard node
flips to not_yet_checked for the tail of its cycle - the exact symptom
the probe cache was added to fix, and the opposite of what the
monitor-loop comment claimed.  The WireGuard skip branch applies no age
gate at all, so the two branches disagreed.

Serve the last reading unconditionally now, matching the WireGuard
branch; the record is simply the node's most recent verdict and the
rotation guarantees it is refreshed every N sweeps.  The monitor-loop
comment is rewritten accordingly and the dead cache_verdict assignment
is dropped (the record no longer carries an ok/failed line).

Signed-off-by: Smth Dagg <smthdagg@gmail.com>
@smthdagg

Copy link
Copy Markdown
Author

Round-38 fix published in 663a34a:

The 60 s gate is gone entirely rather than scaled to the rotation period. Both skip branches (WireGuard and ICMP/TCP) now serve the last reading unconditionally: the cached record is simply the node's most recent verdict, and the rotation guarantees it is refreshed every N sweeps, so a gate could only ever flip big fleets back to not_yet_checked for the tail of a cycle — the symptom the probe cache exists to prevent. The monitor-loop comment is rewritten to match, and the dead cache_verdict assignment is dropped.

Verified locally: 100/100 unit tests (the stale-cache case now asserts the last reading is served), sh -n clean.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit.


Generated by Claude Code

Comment on lines +171 to +173
# Rotated probing: with a target only that node is refreshed; the
# rest report from their 60 s cache (or "not yet checked" on the
# first sweep) instead of being probed all at once.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: this still says "60 s cache", but neither skip branch gates on age any more after 663a34a.

Suggested change
# Rotated probing: with a target only that node is refreshed; the
# rest report from their 60 s cache (or "not yet checked" on the
# first sweep) instead of being probed all at once.
# Rotated probing: with a target only that node is refreshed; the
# rest report their last cached reading (or "not yet checked" on
# the first sweep) instead of being probed all at once.

Generated by Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

add package Introduces a new package Makefile build script

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants