luci-base: fix auth plugin asset loading - #8971
Conversation
This fork's local delta on top of upstream, kept as one commit. build.yml: dispatch-only, and trimmed to the single device and flavour this fork actually builds - the AX3600 on edma-nss. Upstream's other device groups and the mesh/ppe flavours are dropped, not adopted. devices/xiaomi_ax3600: the local package set (dnsmasq-full, nginx-served LuCI, wireguard, collectd/statistics, tcpdump and the shell tooling) and the local radio config. prepare-build.sh: clone the argon theme, and the WebAuthn passkey plugin plus the webauthn-helper binary it shells out to, into package/ and select them all in .config. The helper is selected explicitly because the plugin's Makefile declares +luci-base and nothing else, so nothing would otherwise pull in the binary and the Passkeys page would fail every call with "webauthn-helper binary not found". These out-of-feed selects now go through the same defconfig verification as the device configs, so a silently dropped package stops the build rather than shipping a reduced image. patches/feeds/luci: fix normalize_assets() passing a string to ucode's match(), which requires a regexp and returns null for anything else. Every auth plugin's login-page script is dropped by that filter, which is why the passkey button did nothing. Submitted upstream as openwrt/luci#8971; prepare-build.sh's reverse dry-run makes this patch skip itself once the fix lands.
This fork's local delta on top of upstream, kept as one commit. build.yml: dispatch-only, and trimmed to the single device and flavour this fork actually builds - the AX3600 on edma-nss. Upstream's other device groups and the mesh/ppe flavours are dropped, not adopted. devices/xiaomi_ax3600: the local package set (dnsmasq-full, nginx-served LuCI, wireguard, collectd/statistics, tcpdump and the shell tooling) and the local radio config. prepare-build.sh: clone the argon theme, and the WebAuthn passkey plugin plus the webauthn-helper binary it shells out to, into package/ and select them all in .config. The helper is selected explicitly because the plugin's Makefile declares +luci-base and nothing else, so nothing would otherwise pull in the binary and the Passkeys page would fail every call with "webauthn-helper binary not found". These out-of-feed selects now go through the same defconfig verification as the device configs, so a silently dropped package stops the build rather than shipping a reduced image. patches/feeds/luci: fix normalize_assets() passing a string to ucode's match(), which requires a regexp and returns null for anything else. Every auth plugin's login-page script is dropped by that filter, which is why the passkey button did nothing. Submitted upstream as openwrt/luci#8971; prepare-build.sh's reverse dry-run makes this patch skip itself once the fix lands.
openwrt-ai
left a comment
There was a problem hiding this comment.
The diagnosis checks out. uc_match() bails with if (ucv_type(pattern) != UC_REGEXP || !subject) return NULL; (lib.c:3224), so the string pattern made the check unconditionally false and every asset was dropped. plugin.uuid is indeed only assigned after match(uuid, /^[a-f0-9]{32}$/) passes during load(), so the interpolation-safety claim holds, and regexp() is a core builtin available under 'use strict'. This was also the only match() call in the tree passing a non-regexp pattern, so no sibling instances need the same fix.
Two inline notes, neither blocking.
Generated by Claude Code
8700150 to
1de6e15
Compare
normalize_assets() passes a string to match(), which requires a regexp and returns null for anything else. The path check therefore fails for every asset and each one is dropped by the following continue, so an auth plugin's scripts are never rendered on the login page. The failure is silent. The html returned by the same check() call is unaffected, so a plugin's login UI appears without the script that drives it and nothing is logged. plugins/luci-plugin-auth-example is affected as well. The check is a plain prefix test, so use index() rather than compiling a regexp per asset. It returns the offset of the first occurrence or -1, making != 0 the equivalent of the anchored ^, and comparing two plain strings keeps the check independent of how uuid is validated. Signed-off-by: Fuad Hasan <fuad.antor@gmail.com>
normalize_assets() rejects .. and whitespace but not quotes or angle
brackets, and every sysauth template interpolates the surviving src
straight into an attribute without escaping:
<script src="{{ asset.src }}"></script>
A src of /luci-static/plugins/<uuid>/x.js"></script><script>... passes
both existing checks and closes the attribute. It needs no whitespace,
so the existing character class does not stop it.
The three in-tree templates could entityencode() instead, but
normalize_assets() is the single point every theme goes through, so
filtering here also covers out-of-tree themes carrying the same markup.
src comes from a plugin's own check(), which is server-side ucode able
to emit script through the html field regardless, so this is not a
privilege boundary. It matters for a plugin deriving src from request
input, and it keeps the filter consistent with what it already tries to
do.
Signed-off-by: Fuad Hasan <fuad.antor@gmail.com>
1de6e15 to
b179d85
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Both notes from the previous round are addressed.
The index() switch is correct and is the in-tree idiom for a prefix test. The widened character class parses as intended — inside a regexp literal ucode's lexer reads a bracket expression through to the closing ] and pushes ", ' and ` verbatim (lexer.c:400-412), so no escaping is needed there. Filtering in normalize_assets() rather than in the templates is the right call: all three in-tree sysauth templates (modules/luci-base/ucode/template/sysauth.ut:67, themes/luci-theme-bootstrap/.../sysauth.ut:52, themes/luci-theme-footstrap/.../sysauth.ut:134) carry the same unescaped src="{{ asset.src }}", and out-of-tree themes copying that markup are covered for free. With <, >, ", ' and backtick rejected, the attribute-breakout payload from the earlier round no longer survives, and the fixed /luci-static/plugins/<uuid>/ prefix rules out scheme-based src values.
Both commit messages match their diffs. One nit inline about the PR description having drifted from the code.
Generated by Claude Code
This fork's local delta on top of upstream, kept as one commit. build.yml: dispatch-only, and trimmed to the single device and flavour this fork actually builds - the AX3600 on edma-nss. Upstream's other device groups and the mesh/ppe flavours are dropped, not adopted. devices/xiaomi_ax3600: the local package set (dnsmasq-full, nginx-served LuCI, wireguard, collectd/statistics, tcpdump and the shell tooling) and the local radio config. prepare-build.sh: clone the argon theme, and the WebAuthn passkey plugin plus the webauthn-helper binary it shells out to, into package/ and select them all in .config. The helper is selected explicitly because the plugin's Makefile declares +luci-base and nothing else, so nothing would otherwise pull in the binary and the Passkeys page would fail every call with "webauthn-helper binary not found". These out-of-feed selects now go through the same defconfig verification as the device configs, so a silently dropped package stops the build rather than shipping a reduced image. patches/feeds/luci: fix normalize_assets() passing a string to ucode's match(), which requires a regexp and returns null for anything else. Every auth plugin's login-page script is dropped by that filter, which is why the passkey button did nothing. Submitted upstream as openwrt/luci#8971; prepare-build.sh's reverse dry-run makes this patch skip itself once the fix lands.
normalize_assets()builds its path pattern withsprintf()and hands the resulting string tomatch(). ucode'smatch()only accepts a regexp:It returns null whatever the subject is,
!match(...)is therefore always true, and every asset is dropped by thecontinueunder it. As far as I can tell no auth plugin has ever had its scripts rendered on the login page since the mechanism was added in 4a308ba.It fails silently, which makes it unpleasant to track down. The
htmlfrom the samecheck()result is unaffected, so the plugin's login UI renders normally but without the script that drives it, and nothing is logged.plugins/luci-plugin-auth-exampledeclaresassetstoo, so the shipped example is affected as well.Reproducer:
The check is a plain prefix test, so the first commit uses
index()rather than compiling a regexp inside the loop. It returns the offset of the first occurrence or-1, which makes!= 0the equivalent of the anchored^, and it matches the existing idiom athttp.uc:397.The second commit is a follow-up from review. Because this series is what makes
normalize_assets()return a non-empty result for the first time, the survivingsrcnow reaches<script src="{{ asset.src }}"></script>unescaped in all three in-tree sysauth templates (luci-base,bootstrap,footstrap). The existing filter rejects..and whitespace but not quotes or angle brackets, so asrcendingx.js"></script><script>...would close the attribute — no whitespace required.normalize_assets()is the single point every theme passes through, so the rejected character class is widened there rather than escaping in each template, which also covers out-of-tree themes carrying the same markup.Found while getting a third-party WebAuthn auth plugin working. Verified on qualcommax/ipq807x with luci-base 26.234.21935:
auth_assetsarrives empty at the login template and no<script>tag is emitted, and correcting the path check makes the tag render and the passkey login complete.