Conversation
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>
c486ff7 to
2d86e7a
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
+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
| 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]) "}}" | ||
| } |
There was a problem hiding this comment.
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:
securityis the field that decides this, but there is no form option for it inoverview.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}ininit.d:24.`` A VMess node with a non-emptyflowemits `"alter_id":` and produces invalid JSON.
Generated by Claude Code
| config global 'main' | ||
| option enabled '0' | ||
| option log_level 'warn' | ||
| option monitor_interval '5' |
There was a problem hiding this comment.
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.
| option monitor_interval '5' |
Generated by Claude Code
| 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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
| 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')); |
There was a problem hiding this comment.
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.
| 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 "" | |||
There was a problem hiding this comment.
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>
|
Thanks for the thorough review. All issues addressed in d9c7e7c:
|
openwrt-ai
left a comment
There was a problem hiding this comment.
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.dfixed VMess but left VLESS with${flow:-$alter_id}, andalter_iddefaults to0— so every importer-created VLESS node now compiles to"flow":"0"andsing-box checkrejects 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
parseVmessnever reads the link'stlsfield, 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} ;; |
There was a problem hiding this comment.
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.
| 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]) |
There was a problem hiding this comment.
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.
| 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; } |
There was a problem hiding this comment.
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.
| /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'); |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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>
|
Thanks for the second round. All three issues addressed in d5f9ac4: 1. VLESS flow regression: Changed 2. VMess TLS:
3. Firewall abort cleanup: init.d now calls |
nit fix: value('') rendered as a blank dropdown row; now value('', _('None')).
Signed-off-by: Smth Dagg <smthdagg@gmail.com>
openwrt-ai
left a comment
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| 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]) |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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>
|
Thanks for the third round. All issues addressed in 2192d2e: 1. VMess security not surviving edit: Added 2. Empty server_name: 3. Delimiter guard gap: Added `` to the node delimiter guard ( 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
left a comment
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| 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
| extra="\"enabled\":true" | ||
| if (sni!="") extra=extra ",\"server_name\":" q(sni) | ||
| extra=extra ",\"insecure\":" (insecure=="1"?"true":"false") |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
|
Thanks for the fourth round. All five items addressed in bd32217 (3 files):
|
bd32217 to
81817c6
Compare
- 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>
81817c6 to
60f81a2
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
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.validatecallsthis.map.getSectionValue(), which is not a LuCI API — it throws on every validation of the Security select rather than rejectingrealityon VMess. - The extended delimiter guard still omits
alter_id, which is the one interpolated field fed unvalidated from a pastedvmess://link. A craftedaidshifts the record and re-opens the cleartext-VMess outbound. - Clearing
status.jsoninstop_serviceraces the monitor: rc.common runsstop_servicebeforeprocd_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, viaauxiliary) is still not covered. Same wording appears in the PR comment. Also, that comment attributes the changes tobd32217, 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
| securityOpt.validate = function(section_id, value) { | ||
| if (value == 'reality' && this.map.getSectionValue(section_id, 'protocol') == 'vmess') | ||
| return false; | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
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".
| 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 |
There was a problem hiding this comment.
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.
| 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
| 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 | ||
| } |
There was a problem hiding this comment.
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.
| 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]) |
There was a problem hiding this comment.
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
- 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>
4643c90 to
ab5366c
Compare
|
Pushed
Local checks before push: 40/40 unit tests pass, |
- 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>
bd498d8 to
e1080ff
Compare
|
Round-30 finding addressed in
All three FormalityCheck runs are green on |
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
e1080ff7"fix round-30, id whitelist (1.9.7)" — the subject announces 1.9.7, butPKG_VERSIONis still1.9.6and 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 spurioushandshake_success/handshake_failed;handshake_failedrequiredold_wfcto beregistered/connecting, so it could never fire on a pristine state. Onlyhandshake_successwas affected.
Generated by Claude Code
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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
There was a problem hiding this comment.
| 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'), ''); }); |
There was a problem hiding this comment.
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.
| 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
There was a problem hiding this comment.
| printf '{"generated_at":%s,"nodes":[' "$(date +%s)" | ||
| first=1 | ||
| while IFS='|' read -r id label protocol server port; do | ||
| [ -n "$id" ] || continue |
There was a problem hiding this comment.
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.
| [ -n "$id" ] || continue | |
| case "$id" in ''|*[!A-Za-z0-9_]*) continue;; esac |
Generated by Claude Code
There was a problem hiding this comment.
| # 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
|
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 What I can check locally on
|
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>
|
Root cause of the Test Build failure found and fixed in The SDK's package scan ( This is invisible to the local harness ( The runs on |
| # luci.mk assigns the main package's Maintainer from LUCI_MAINTAINER | ||
| # (default: "OpenWrt LuCI community"); set it so the author is credited. |
There was a problem hiding this comment.
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.
| # 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
There was a problem hiding this comment.
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>
|
WLOC 1.4 wificalling component synced in
Every standalone review fix is kept (old_hs debounce, pristine-baseline guard, WG handshake verification, acl additions). Verified locally: 90/90 unit tests, |
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
left a comment
There was a problem hiding this comment.
Commit checks
- 55205b1 "sync the WLOC 1.4 wificalling component" — the body lists the
passwall-bypass.shhunk as a "command simplification", but it adds a chain-existence guard that changes behaviour (the helper now skips a missinginet passwallchain 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) |
There was a problem hiding this comment.
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.
| 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
There was a problem hiding this comment.
| 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') } } |
There was a problem hiding this comment.
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.
| set macs { type ether_addr; elements = { $(printf '%s' "$macs" | sed 's/,/, /g') } } | |
| set macs { type ether_addr; ${macs:+elements = { $macs }} } |
Generated by Claude Code
There was a problem hiding this comment.
fixed, thanks — solved with an explicit $mac_set fallback rather than the inline ${macs:+…}.
Generated by Claude Code
| _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 }') |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
| 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'); } |
There was a problem hiding this comment.
/tmp/wg-health-$idis written only bywg_handshake_test, so for ICMP/TCP nodes it never exists and the skip branch always falls tonot_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_jsonis not reset anywhere in this branch, so a node taking theokarm 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) |
There was a problem hiding this comment.
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.
| anytls|vless|vmess|trojan) | |
| anytls|vless|vmess|trojan|shadowsocks) |
Generated by Claude Code
There was a problem hiding this comment.
| if (!value) return true; | ||
| return (ssMethods.indexOf(value) >= 0) ? true : _('Unsupported Shadowsocks encryption method'); |
There was a problem hiding this comment.
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.
| 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
There was a problem hiding this comment.
| /usr/libexec/wificalling-gateway/node-health.sh "$nodes" "$node_output" "$node" | ||
| printf '%s\n' "$node" > "$cursor" | ||
| fi | ||
| tick=$(( (tick + 1) % 2 )) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
| 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). |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
… 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>
|
Round-36 fixes published in aecd37e:
Noted on 55205b1: the passwall-bypass.sh hunk is indeed a chain-existence guard (a missing Verified locally: 99/99 unit tests (5 new covering the MAC join, the empty-set fallback and the rotation cache paths), |
openwrt-ai
left a comment
There was a problem hiding this comment.
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"lladdrat field 2, butip neigh show <ip> dev <dev>suppresses thedev DEVpair, which putslladdrat field 2 and made the old$2 == "lladdr"the matching form. Was the AX6S check run without thedevfilter? 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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
|
Round-37 fix published in e6ba762: The rotated skip branch now reads a dedicated Noted on aecd37e: the stock OpenWrt neighbour table ( Verified locally: 100/100 unit tests (probe-path assertions + an expired-cache case), |
… 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>
ac150a6 to
f7cfca2
Compare
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
nit: cache_verdict is dead now that the record no longer carries an ok/failed line.
| if [ "$state" = reachable ] || [ "$state" = tcp_reachable ]; then cache_verdict=ok; else cache_verdict=failed; fi |
Generated by Claude Code
There was a problem hiding this comment.
…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>
|
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 Verified locally: 100/100 unit tests (the stale-cache case now asserts the last reading is served), |
| # 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. |
There was a problem hiding this comment.
nit: this still says "60 s cache", but neither skip branch gates on age any more after 663a34a.
| # 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
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:
independentroutes through the node,follow_gatewayis not intercepted.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.anytls://,hy2://,tuic://,vless://,vmess://,trojan://,wg://parsed locally in the browser.tcpingfallback for TCP-based protocols when installed (no hard dependency).sing-box checkbefore startup; mode0600for 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?
sh -n/node --check/git diff --checkclean.sing-box checkagainst real 1.10.0 / 1.11.7 / 1.12.0 / 1.13.18 binaries for both wireguard forms.Checklist