luci-app-frps: align dashboard options with frps init script - #8987
luci-app-frps: align dashboard options with frps init script#8987frank-pv wants to merge 1 commit into
Conversation
Formality Check: FailedWe checked this pull request against the contribution guidelines. Here is what needs your attention: 🛑 CRITICAL ERRORS
Tip Do not close this pull request to make corrections. Instead, modify your existing commits (e.g. Something broken? Consider reporting an issue. |
openwrt-ai
left a comment
There was a problem hiding this comment.
The core premise checks out: frps.init reads the admin_* options exclusively (_emit_admin_web / _emit_admin_web_tls), the dashboard_* names appear only in frps.uci-defaults as one-shot migration sources, and frps.config ships the admin_* spelling — so the LuCI page really was writing dead options. The admin_tls_mode → admin_tls_enable mapping and the .toml include requirement both match the init script as well. Backend consumers verified against openwrt/packages@ff5519f.
Five inline notes: one blocking-ish bug on the TLS file fields (an invalid datatype that predates this PR but sits on lines being rewritten), one factual correction to a default value, and three nits.
Generated by Claude Code
| [form.Value, 'admin_tls_cert_file', _('Dashboard TLS certificate'), _('Dashboard TLS certificate specifies the path to the TLS certificate file for enabling HTTPS access.<br />Required if HTTPS is enabled.'), {datatype: 'filepath'}], | ||
| [form.Value, 'admin_tls_key_file', _('Dashboard TLS private key'), _('Dashboard TLS private key specifies the path to the TLS private key file for enabling HTTPS access.<br />Required if HTTPS is enabled.'), {datatype: 'filepath'}], |
There was a problem hiding this comment.
filepath is not a registered LuCI datatype, so these two fields can never be saved. ValidatorFactory.compile() looks the token up in types and raises SyntaxError: Unhandled token "filepath" when it isn't a function — nothing in validation.js defines filepath. These are the only two occurrences of the token in the whole tree; the datatype LuCI provides for path strings is file.
The suggestion also converts the leading 4 spaces on the second line to a tab, matching every other entry in the array.
| [form.Value, 'admin_tls_cert_file', _('Dashboard TLS certificate'), _('Dashboard TLS certificate specifies the path to the TLS certificate file for enabling HTTPS access.<br />Required if HTTPS is enabled.'), {datatype: 'filepath'}], | |
| [form.Value, 'admin_tls_key_file', _('Dashboard TLS private key'), _('Dashboard TLS private key specifies the path to the TLS private key file for enabling HTTPS access.<br />Required if HTTPS is enabled.'), {datatype: 'filepath'}], | |
| [form.Value, 'admin_tls_cert_file', _('Dashboard TLS certificate'), _('Dashboard TLS certificate specifies the path to the TLS certificate file for enabling HTTPS access.<br />Required if HTTPS is enabled.'), {datatype: 'file'}], | |
| [form.Value, 'admin_tls_key_file', _('Dashboard TLS private key'), _('Dashboard TLS private key specifies the path to the TLS private key file for enabling HTTPS access.<br />Required if HTTPS is enabled.'), {datatype: 'file'}], |
Generated by Claude Code
| [form.Flag, 'dashboard_tls_mode', _('Dashboard TLS mode'), _('Enable or disable TLS encryption for the dashboard. When enabled, HTTPS is used for secure communication.'), {datatype: 'bool'}], | ||
| [form.Value, 'dashboard_tls_cert_file', _('Dashboard TLS certificate'), _('Dashboard TLS Cert File specifies the path to the TLS certificate file for enabling HTTPS access.<br />Required if HTTPS is enabled.'), {datatype: 'filepath'}], | ||
| [form.Value, 'dashboard_tls_key_file', _('Dashboard TLS private key'), _('Dashboard TLS Key File specifies the path to the TLS private key file for enabling HTTPS access.<br />Required if HTTPS is enabled.'), {datatype: 'filepath'}], | ||
| [form.Value, 'admin_addr', _('Dashboard address'), _('Dashboard address specifies the address that the dashboard binds to.<br />By default, this value is "0.0.0.0".'), {datatype: 'ipaddr'}], |
There was a problem hiding this comment.
The stated default is wrong for the init script this PR is aligning to. _emit_admin_web() falls back to 127.0.0.1, not 0.0.0.0, when admin_addr is unset: _toml_line webServer.addr "${addr:-127.0.0.1}" string at frps.init:452. The shipped frps.config also comments the option out as 127.0.0.1. The 0.0.0.0 figure was carried over from the old frp .ini DashboardAddr semantics.
| [form.Value, 'admin_addr', _('Dashboard address'), _('Dashboard address specifies the address that the dashboard binds to.<br />By default, this value is "0.0.0.0".'), {datatype: 'ipaddr'}], | |
| [form.Value, 'admin_addr', _('Dashboard address'), _('Dashboard address specifies the address that the dashboard binds to.<br />By default, this value is "127.0.0.1".'), {datatype: 'ipaddr'}], |
Generated by Claude Code
| @@ -25,13 +25,13 @@ const commonConf = [ | |||
| [form.Value, 'vhost_http_port', _('Vhost HTTP port'), _('VhostHttpPort specifies the port that the server listens for HTTP Vhost requests. If this value is 0, the server will not listen for HTTP requests.<br />By default, this value is 0.'), {datatype: 'port'}], | |||
There was a problem hiding this comment.
nit: while auditing this page against frps.init, one more option in the same array has no backend consumer — bind_udp_port on line 21. Grepping the whole net/frp package for bind_udp_port / bindUdpPort / bindUDPPort returns nothing: _emit_common() emits bind_addr, bind_port, kcp_bind_port, quic_bind_port and no UDP bind port, and frps.config never mentions it. frp dropped bind_udp_port when it moved to TOML, so this field is silently ignored the same way the dashboard_* names were.
Was leaving it in place deliberate (out of scope for this commit), or should it be dropped here too?
Generated by Claude Code
| [form.Value, 'dashboard_tls_key_file', _('Dashboard TLS private key'), _('Dashboard TLS Key File specifies the path to the TLS private key file for enabling HTTPS access.<br />Required if HTTPS is enabled.'), {datatype: 'filepath'}], | ||
| [form.Value, 'admin_addr', _('Dashboard address'), _('Dashboard address specifies the address that the dashboard binds to.<br />By default, this value is "0.0.0.0".'), {datatype: 'ipaddr'}], | ||
| [form.Value, 'admin_port', _('Dashboard port'), _('Dashboard port specifies the port that the dashboard listens on. If this value is 0, the dashboard will not be started.<br />By default, this value is 0.'), {datatype: 'port'}], | ||
| [form.Value, 'admin_user', _('Dashboard user'), _('Dashboard user specifies the username that the dashboard will use for login.<br />By default, this value is "admin".')], |
There was a problem hiding this comment.
nit: this description rewrite (DashboardUser specifies… → Dashboard user specifies…) is purely cosmetic — the UCI option rename is what fixes the bug, the help string is not read by anything. Changing the msgid invalidates the existing translations: 42 po/*/frps.po files carry these strings today (e.g. po/zh_Hans/frps.po has a filled-in msgstr for this one), and they all revert to English on merge. Same applies to the description-only edits on lines 29, 31, 33 and 34.
If the rewording is kept, it would be more useful to name the TOML key the init script actually generates (webServer.user here, webServer.addr / webServer.port / webServer.password above) rather than repeating the field label — that gives users something greppable against frps_full_example.toml.
Generated by Claude Code
| [form.Flag, 'respawn', _('Respawn when crashed')], | ||
| [form.DynamicList, 'env', _('Environment variable'), _('OS environments pass to frp for config file template, see %s'.format(`<a href="https://github.com/fatedier/frp#configuration-file-template">frp README</a>`)), {placeholder: 'ENV_NAME=value'}], | ||
| [form.DynamicList, 'conf_inc', _('Additional configs'), _('Config files include in temporary config file'), {placeholder: '/etc/frp/frps.d/frps_full.ini'}] | ||
| [form.DynamicList, 'conf_inc', _('Additional configs'), _('Config files include in temporary config file'), {placeholder: '/etc/frp/frps.d/frps_full.toml'}] |
There was a problem hiding this comment.
nit: the .ini → .toml fix is correct here (_append_conf_file() rejects anything not ending in .toml under /etc/frp/frps.d/), but the sibling app has the identical stale placeholder — luci-app-frpc/htdocs/luci-static/resources/view/frpc.js:16 still suggests /etc/frp/frpc.d/frpc_full.ini, and frpc.init applies the same "$dir"*.toml guard. Worth fixing in the same series so the two pages stay in step.
(For what it's worth, luci-app-frpc already uses admin_addr/admin_port/admin_user/admin_pwd, so the rename in this PR brings frps into parity with frpc rather than diverging from it — the direction looks right.)
Generated by Claude Code
|
Forget that one—try this one instead: #8674 |
Pull request details
Description
Align the LuCI frps configuration page with the option names consumed by
frps.init.The init script uses the
admin_*UCI options when generating the FRP TOMLconfiguration, while the LuCI page previously saved the dashboard settings
using
dashboard_*option names. This caused dashboard settings changed inLuCI to be ignored by the service.
This change:
admin_*names:admin_addradmin_portadmin_useradmin_pwdadmin_tls_enableadmin_tls_cert_fileadmin_tls_key_file.inito.toml.frps.uci-defaultsmigration logic.Screenshot or video of changes
Not applicable.
Maintainer
@
Tested on
OpenWrt version: Not tested on a running OpenWrt system
LuCI version: LuCI master
Web browser(s): Not tested
Checklist