luci-app-pbr: preserve resolver_set selection - #8927
Conversation
Since commit 974b586 ("luci-base: fix uci write bug when input value equals default"), form values matching their default are removed when rmempty is enabled. luci-app-pbr selects the resolver_set default dynamically according to the resolver support available on the device. Unlike the other PBR options with defaults, pbr treats a missing resolver_set as disabled. Saving the page can therefore silently disable dnsmasq nft set handling and make domain policies fall back to one-time resolveip lookups. Use "none" as the base default and disable rmempty so LuCI always stores the selected resolver. Signed-off-by: Andrea Pesaresi <andreapesaresi82@gmail.com>
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; one question on the interaction between the new rmempty = false and the platform-conditional defaults. The commit message matches its diff, the PKG_RELEASE bump is present, and the resolver_set option is confirmed to have a backend consumer in net/pbr (openwrt/packages).
Generated by Claude Code
REQUIRES luci-base commit 974b5864e05ef30f38149389f15583c08bdd4eda
("luci-base: form: do not write values equal to the default").
That commit changed CBIAbstractValue.parse() in
modules/luci-base/htdocs/luci-static/resources/form.js to:
if (fval == null || fval == '' ||
(fval == this.default && (this.optional || this.rmempty))) {
... remove() ...
}
else if (this.forcewrite || !isEqual(cval, fval)) {
... write() ...
}
An option whose form value equals its declared o.default is now REMOVED
from UCI rather than written, whenever optional or rmempty is true. Both
rmempty (true) and the removal clause are on by default, so this silently
applies to nearly every option that does not explicitly opt out. It also
makes o.forcewrite = true dead code in precisely the case it is usually
added for: forcewrite lives in the else-if branch, which the new first
condition now short-circuits.
Changes here:
* resolver_set: rmempty = false. This is the only functional fix. pbr's
backend treats an absent resolver_set as disabled
(files/lib/pbr/platform.uc: resolver_set_supported is true when the
value is empty or 'none'), while LuCI declares a default of
'dnsmasq.nftset' on capable devices.
This is not merely a default failing to be written. pbr ships
resolver_set 'dnsmasq.nftset' in files/etc/config/pbr, and
files/etc/uci-defaults/90-pbr force-sets it on every install and upgrade
(nftset when dnsmasq supports it, 'none' otherwise), so the option is
present in practice on every device. After 974b5864 the first Save of
the overview page therefore DELETES that provisioned value, and because
90-pbr only re-runs on package upgrade, nft set handling stays off until
then: the UI keeps showing nft set support enabled while pbr runs with
it off and domain policies fall back to one-time resolveip lookups.
An explicit o.default = 'none' base is set as well: it is inert while
rmempty and optional are both false, but it documents the fallback on
devices without nft set support and keeps the option correct if rmempty
is ever restored.
* uplink_interface / uplink_interface6: rmempty = false, and the now-dead
forcewrite = true removed. pbr falls back to wan/wan6 when these are
absent, so this is visibility only -- the declared defaults are written
to /etc/config/pbr so users can see and edit them. Note that
uplink_interface6 is still removed when ipv6_enabled is false; that is
the inactive-depends() path (else if (!this.retain) remove()), which is
unconditional and unrelated to rmempty.
Safe on older branches that do not have 974b5864: the upstream change is a
single added clause, so on old form.js these options behave exactly as
before. resolver_set is a ListValue with no blank choice, so rmempty =
false is never reached; and the dropped forcewrite only ever forced a
rewrite of an already-identical value, since an absent option has cval ==
null and is written on the inequality check anyway. The one visible
difference on any branch is that the uplink_interface fields can no longer
be cleared to blank, which is intended. No config churn on up- or
downgrade.
Wider impact -- this luci-base change can break more than pbr:
* Any app that declares o.default and depends on that value actually
reaching /etc/config now gets nothing written. If the package's own
fallback differs from the LuCI default, the UI and the running service
silently disagree. That is exactly the resolver_set case above, and it
is invisible until something misbehaves at runtime.
* Any existing o.forcewrite = true added to force a default into the
config is now a no-op. The correct replacement is rmempty = false, since
cfgvalue() returns null for an absent option.
* rmempty = false additionally makes the field mandatory, so it is not a
drop-in fix for free-text form.Value fields where a blank entry used to
be legitimate. Fields with optional = true need optional cleared as
well, since optional arms the same removal clause independently.
Other pbr views, and other LuCI apps generally, likely need the same
audit.
Thanks to @pesa1234 for bringing this to our attention and for reviewing
the resolver_set analysis. The equivalent fix for 1.2.2 is openwrt/luci
PR #8927 ("luci-app-pbr: preserve resolver_set selection"); that version
carries three resolver branches instead of one, but the fix is the same:
openwrt/luci#8927 (comment)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
REQUIRES luci-base commit 974b5864e05ef30f38149389f15583c08bdd4eda
("luci-base: form: do not write values equal to the default").
That commit changed CBIAbstractValue.parse() in
modules/luci-base/htdocs/luci-static/resources/form.js to:
if (fval == null || fval == '' ||
(fval == this.default && (this.optional || this.rmempty))) {
... remove() ...
}
else if (this.forcewrite || !isEqual(cval, fval)) {
... write() ...
}
An option whose form value equals its declared o.default is now REMOVED
from UCI rather than written, whenever optional or rmempty is true. Both
rmempty (true) and the removal clause are on by default, so this silently
applies to nearly every option that does not explicitly opt out. It also
makes o.forcewrite = true dead code in precisely the case it is usually
added for: forcewrite lives in the else-if branch, which the new first
condition now short-circuits.
Changes here:
* resolver_set: rmempty = false. This is the only functional fix. pbr's
backend treats an absent resolver_set as disabled
(files/lib/pbr/platform.uc: resolver_set_supported is true when the
value is empty or 'none'), while LuCI declares a default of
'dnsmasq.nftset' on capable devices.
This is not merely a default failing to be written. pbr ships
resolver_set 'dnsmasq.nftset' in files/etc/config/pbr, and
files/etc/uci-defaults/90-pbr force-sets it on every install and upgrade
(nftset when dnsmasq supports it, 'none' otherwise), so the option is
present in practice on every device. After 974b5864 the first Save of
the overview page therefore DELETES that provisioned value, and because
90-pbr only re-runs on package upgrade, nft set handling stays off until
then: the UI keeps showing nft set support enabled while pbr runs with
it off and domain policies fall back to one-time resolveip lookups.
An explicit o.default = 'none' base is set as well: it is inert while
rmempty and optional are both false, but it documents the fallback on
devices without nft set support and keeps the option correct if rmempty
is ever restored.
* uplink_interface / uplink_interface6: rmempty = false, and the now-dead
forcewrite = true removed. pbr falls back to wan/wan6 when these are
absent, so this is visibility only -- the declared defaults are written
to /etc/config/pbr so users can see and edit them. Note that
uplink_interface6 is still removed when ipv6_enabled is false; that is
the inactive-depends() path (else if (!this.retain) remove()), which is
unconditional and unrelated to rmempty.
Safe on older branches that do not have 974b5864: the upstream change is a
single added clause, so on old form.js these options behave exactly as
before. resolver_set is a ListValue with no blank choice, so rmempty =
false is never reached; and the dropped forcewrite only ever forced a
rewrite of an already-identical value, since an absent option has cval ==
null and is written on the inequality check anyway. The one visible
difference on any branch is that the uplink_interface fields can no longer
be cleared to blank, which is intended. No config churn on up- or
downgrade.
Wider impact -- this luci-base change can break more than pbr:
* Any app that declares o.default and depends on that value actually
reaching /etc/config now gets nothing written. If the package's own
fallback differs from the LuCI default, the UI and the running service
silently disagree. That is exactly the resolver_set case above, and it
is invisible until something misbehaves at runtime.
* Any existing o.forcewrite = true added to force a default into the
config is now a no-op. The correct replacement is rmempty = false, since
cfgvalue() returns null for an absent option.
* rmempty = false additionally makes the field mandatory, so it is not a
drop-in fix for free-text form.Value fields where a blank entry used to
be legitimate. Fields with optional = true need optional cleared as
well, since optional arms the same removal clause independently.
Other pbr views, and other LuCI apps generally, likely need the same
audit.
Thanks to @pesa1234 for bringing this to our attention and for reviewing
the resolver_set analysis. The equivalent fix for 1.2.2 is openwrt/luci
PR #8927 ("luci-app-pbr: preserve resolver_set selection"); that version
carries three resolver branches instead of one, but the fix is the same:
openwrt/luci#8927 (comment)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
REQUIRES luci-base commit 974b5864e05ef30f38149389f15583c08bdd4eda
("luci-base: form: do not write values equal to the default").
That commit changed CBIAbstractValue.parse() in
modules/luci-base/htdocs/luci-static/resources/form.js to:
if (fval == null || fval == '' ||
(fval == this.default && (this.optional || this.rmempty))) {
... remove() ...
}
else if (this.forcewrite || !isEqual(cval, fval)) {
... write() ...
}
An option whose form value equals its declared o.default is now REMOVED
from UCI rather than written, whenever optional or rmempty is true. Both
rmempty (true) and the removal clause are on by default, so this silently
applies to nearly every option that does not explicitly opt out. It also
makes o.forcewrite = true dead code in precisely the case it is usually
added for: forcewrite lives in the else-if branch, which the new first
condition now short-circuits.
Changes here:
* resolver_set: rmempty = false. This is the only functional fix. pbr's
backend treats an absent resolver_set as disabled
(files/lib/pbr/platform.uc: resolver_set_supported is true when the
value is empty or 'none'), while LuCI declares a default of
'dnsmasq.nftset' on capable devices.
This is not merely a default failing to be written. pbr ships
resolver_set 'dnsmasq.nftset' in files/etc/config/pbr, and
files/etc/uci-defaults/90-pbr force-sets it on every install and upgrade
(nftset when dnsmasq supports it, 'none' otherwise), so the option is
present in practice on every device. After 974b5864 the first Save of
the overview page therefore DELETES that provisioned value, and because
90-pbr only re-runs on package upgrade, nft set handling stays off until
then: the UI keeps showing nft set support enabled while pbr runs with
it off and domain policies fall back to one-time resolveip lookups.
An explicit o.default = 'none' base is set as well: it is inert while
rmempty and optional are both false, but it documents the fallback on
devices without nft set support and keeps the option correct if rmempty
is ever restored.
* uplink_interface / uplink_interface6: rmempty = false, and the now-dead
forcewrite = true removed. pbr falls back to wan/wan6 when these are
absent, so this is visibility only -- the declared defaults are written
to /etc/config/pbr so users can see and edit them. Note that
uplink_interface6 is still removed when ipv6_enabled is false; that is
the inactive-depends() path (else if (!this.retain) remove()), which is
unconditional and unrelated to rmempty.
Safe on older branches that do not have 974b5864: the upstream change is a
single added clause, so on old form.js these options behave exactly as
before. resolver_set is a ListValue with no blank choice, so rmempty =
false is never reached; and the dropped forcewrite only ever forced a
rewrite of an already-identical value, since an absent option has cval ==
null and is written on the inequality check anyway. The one visible
difference on any branch is that the uplink_interface fields can no longer
be cleared to blank, which is intended. No config churn on up- or
downgrade.
Wider impact -- this luci-base change can break more than pbr:
* Any app that declares o.default and depends on that value actually
reaching /etc/config now gets nothing written. If the package's own
fallback differs from the LuCI default, the UI and the running service
silently disagree. That is exactly the resolver_set case above, and it
is invisible until something misbehaves at runtime.
* Any existing o.forcewrite = true added to force a default into the
config is now a no-op. The correct replacement is rmempty = false, since
cfgvalue() returns null for an absent option.
* rmempty = false additionally makes the field mandatory, so it is not a
drop-in fix for free-text form.Value fields where a blank entry used to
be legitimate. Fields with optional = true need optional cleared as
well, since optional arms the same removal clause independently.
Other pbr views, and other LuCI apps generally, likely need the same
audit.
Thanks to @pesa1234 for bringing this to our attention and for reviewing
the resolver_set analysis. The equivalent fix for 1.2.2 is openwrt/luci
PR #8927 ("luci-app-pbr: preserve resolver_set selection"); that version
carries three resolver branches instead of one, but the fix is the same:
openwrt/luci#8927 (comment)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for bringing this to our attention. The Luci-Base "fix" causing this is luckily only in Master branch as far as I can tell and is hopefully not backported as it will cause more problems when Luci and back-end have different defaults. Your PR is against version 1.2.2 which is EOL as we are working on an ucode port 1.2.3 so changes will be made there and I am working on it as we speak. Again many thanks |
…tings on save (#35) * luci-app-pbr-1.2.3: adapt to luci-base default-vs-write UCI change REQUIRES luci-base commit 974b5864e05ef30f38149389f15583c08bdd4eda ("luci-base: form: do not write values equal to the default"). That commit changed CBIAbstractValue.parse() in modules/luci-base/htdocs/luci-static/resources/form.js to: if (fval == null || fval == '' || (fval == this.default && (this.optional || this.rmempty))) { ... remove() ... } else if (this.forcewrite || !isEqual(cval, fval)) { ... write() ... } An option whose form value equals its declared o.default is now REMOVED from UCI rather than written, whenever optional or rmempty is true. Both rmempty (true) and the removal clause are on by default, so this silently applies to nearly every option that does not explicitly opt out. It also makes o.forcewrite = true dead code in precisely the case it is usually added for: forcewrite lives in the else-if branch, which the new first condition now short-circuits. Changes here: * resolver_set: rmempty = false. This is the only functional fix. pbr's backend treats an absent resolver_set as disabled (files/lib/pbr/platform.uc: resolver_set_supported is true when the value is empty or 'none'), while LuCI declares a default of 'dnsmasq.nftset' on capable devices. This is not merely a default failing to be written. pbr ships resolver_set 'dnsmasq.nftset' in files/etc/config/pbr, and files/etc/uci-defaults/90-pbr force-sets it on every install and upgrade (nftset when dnsmasq supports it, 'none' otherwise), so the option is present in practice on every device. After 974b5864 the first Save of the overview page therefore DELETES that provisioned value, and because 90-pbr only re-runs on package upgrade, nft set handling stays off until then: the UI keeps showing nft set support enabled while pbr runs with it off and domain policies fall back to one-time resolveip lookups. An explicit o.default = 'none' base is set as well: it is inert while rmempty and optional are both false, but it documents the fallback on devices without nft set support and keeps the option correct if rmempty is ever restored. * uplink_interface / uplink_interface6: rmempty = false, and the now-dead forcewrite = true removed. pbr falls back to wan/wan6 when these are absent, so this is visibility only -- the declared defaults are written to /etc/config/pbr so users can see and edit them. Note that uplink_interface6 is still removed when ipv6_enabled is false; that is the inactive-depends() path (else if (!this.retain) remove()), which is unconditional and unrelated to rmempty. Safe on older branches that do not have 974b5864: the upstream change is a single added clause, so on old form.js these options behave exactly as before. resolver_set is a ListValue with no blank choice, so rmempty = false is never reached; and the dropped forcewrite only ever forced a rewrite of an already-identical value, since an absent option has cval == null and is written on the inequality check anyway. The one visible difference on any branch is that the uplink_interface fields can no longer be cleared to blank, which is intended. No config churn on up- or downgrade. Wider impact -- this luci-base change can break more than pbr: * Any app that declares o.default and depends on that value actually reaching /etc/config now gets nothing written. If the package's own fallback differs from the LuCI default, the UI and the running service silently disagree. That is exactly the resolver_set case above, and it is invisible until something misbehaves at runtime. * Any existing o.forcewrite = true added to force a default into the config is now a no-op. The correct replacement is rmempty = false, since cfgvalue() returns null for an absent option. * rmempty = false additionally makes the field mandatory, so it is not a drop-in fix for free-text form.Value fields where a blank entry used to be legitimate. Fields with optional = true need optional cleared as well, since optional arms the same removal clause independently. Other pbr views, and other LuCI apps generally, likely need the same audit. Thanks to @pesa1234 for bringing this to our attention and for reviewing the resolver_set analysis. The equivalent fix for 1.2.2 is openwrt/luci PR #8927 ("luci-app-pbr: preserve resolver_set selection"); that version carries three resolver branches instead of one, but the fix is the same: openwrt/luci#8927 (comment) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * luci-app-pbr-1.2.3: keep resolver_set when nft set support is undetected The resolver_set select only offers the dnsmasq.nftset choice when reply.platform.dnsmasq_nftset_support is true. That flag comes from files/lib/pbr/platform.uc, which leaves dnsmasq_nftset_supported at false unless dnsmasq is installed and its compile-time options can be probed, so detection fails transiently on a device where dnsmasq is missing, has just been removed, or is not yet running when the page loads. In that state the stored value is not among the choices. UISelect.render() marks an <option> selected only for values present in the choice list, so nothing is selected and the browser falls back to the first entry; UISelect.getValue() returns node.firstChild.value and therefore reports 'none'. Saving the page then writes resolver_set 'none' and silently disables nft set handling, with domain policies falling back to one-time resolveip lookups. The service keeps running, so nothing points at the overview page as the cause. Re-add dnsmasq.nftset as a selectable choice when it is the stored value, even if support is currently not detected. The value then round-trips untouched: cfgvalue equals formvalue, isEqual() matches and parse() writes nothing. o.default stays 'none', which is only consulted when the option is absent. The choice reuses the existing "Dnsmasq nft set" string, adding no new msgid. No annotation is needed on the entry itself because the field description is built immediately above and already reports "The dnsmasq.nftset is not supported on this system." (or "... support is unknown.") in exactly this branch. This bug predates 974b5864e05e and is independent of it, but it defeats the same protection: one is a value removed on save, the other a value downgraded on save. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Backport of the resolver_set part of the 1.2.3 fix (mossdef-org#35) to the 1.2.2 branch. REQUIRES luci-base commit 974b5864e05ef30f38149389f15583c08bdd4eda ("luci-base: form: do not write values equal to the default"), which changed CBIAbstractValue.parse() in modules/luci-base/htdocs/luci-static/resources/form.js to: if (fval == null || fval == '' || (fval == this.default && (this.optional || this.rmempty))) { ... remove() ... } else if (this.forcewrite || !isEqual(cval, fval)) { ... write() ... } An option whose form value equals its declared o.default is now REMOVED from UCI rather than written, whenever optional or rmempty is true. rmempty defaults to true, so this silently applies to nearly every option that does not explicitly opt out. resolver_set is affected: pbr's backend treats an absent resolver_set as disabled (files/etc/init.d/pbr unsets it when empty or 'none'), while LuCI declares a default of adguardhome.ipset / dnsmasq.ipset / dnsmasq.nftset depending on detected support. pbr ships resolver_set 'dnsmasq.nftset' in files/etc/config/pbr, so after 974b5864 the first Save of the overview page DELETES that provisioned value and resolver set handling is silently disabled: the UI keeps showing resolver set support enabled while pbr runs with it off and domain policies fall back to one-time resolveip lookups. Unlike 1.2.3, the 1.2.2 files/etc/uci-defaults/90-pbr does not re-set resolver_set (it only renames the old resolver_ipset option), so nothing restores the value on a later package upgrade -- once deleted it stays deleted. o.rmempty = false is the fix; cfgvalue() returns null for an absent option, so it forces the write. An explicit o.default = 'none' base is set as well: it is inert while rmempty and optional are both false, but it documents the fallback on devices without any resolver set support. Not backported from the 1.2.3 change: the uplink_interface and uplink_interface6 adjustments, because those options do not exist in the 1.2.2 overview, and the forcewrite removal, because 1.2.2 does not use forcewrite anywhere. Safe on older luci-base without 974b5864: resolver_set is a ListValue with no blank choice, so rmempty = false is never reached. No config churn on up- or downgrade. Thanks to @pesa1234 for spotting this and for the equivalent fix in the openwrt/luci tree, PR #8927 ("luci-app-pbr: preserve resolver_set selection"): openwrt/luci#8927 (comment) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Backport of the resolver_set part of the 1.2.3 fix (#35) to the 1.2.2 branch. REQUIRES luci-base commit 974b5864e05ef30f38149389f15583c08bdd4eda ("luci-base: form: do not write values equal to the default"), which changed CBIAbstractValue.parse() in modules/luci-base/htdocs/luci-static/resources/form.js to: if (fval == null || fval == '' || (fval == this.default && (this.optional || this.rmempty))) { ... remove() ... } else if (this.forcewrite || !isEqual(cval, fval)) { ... write() ... } An option whose form value equals its declared o.default is now REMOVED from UCI rather than written, whenever optional or rmempty is true. rmempty defaults to true, so this silently applies to nearly every option that does not explicitly opt out. resolver_set is affected: pbr's backend treats an absent resolver_set as disabled (files/etc/init.d/pbr unsets it when empty or 'none'), while LuCI declares a default of adguardhome.ipset / dnsmasq.ipset / dnsmasq.nftset depending on detected support. pbr ships resolver_set 'dnsmasq.nftset' in files/etc/config/pbr, so after 974b5864 the first Save of the overview page DELETES that provisioned value and resolver set handling is silently disabled: the UI keeps showing resolver set support enabled while pbr runs with it off and domain policies fall back to one-time resolveip lookups. Unlike 1.2.3, the 1.2.2 files/etc/uci-defaults/90-pbr does not re-set resolver_set (it only renames the old resolver_ipset option), so nothing restores the value on a later package upgrade -- once deleted it stays deleted. o.rmempty = false is the fix; cfgvalue() returns null for an absent option, so it forces the write. An explicit o.default = 'none' base is set as well: it is inert while rmempty and optional are both false, but it documents the fallback on devices without any resolver set support. Not backported from the 1.2.3 change: the uplink_interface and uplink_interface6 adjustments, because those options do not exist in the 1.2.2 overview, and the forcewrite removal, because 1.2.2 does not use forcewrite anywhere. Safe on older luci-base without 974b5864: resolver_set is a ListValue with no blank choice, so rmempty = false is never reached. No config churn on up- or downgrade. Thanks to @pesa1234 for spotting this and for the equivalent fix in the openwrt/luci tree, PR #8927 ("luci-app-pbr: preserve resolver_set selection"): openwrt/luci#8927 (comment) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Since commit 974b586 ("luci-base: fix uci write bug when input value equals default"), form values matching their default are removed when rmempty is enabled.
luci-app-pbr selects the resolver_set default dynamically according to the resolver support available on the device. Unlike the other PBR options with defaults, pbr treats a missing resolver_set as disabled.
Saving the page can therefore silently disable dnsmasq nft set handling and make domain policies fall back to one-time resolveip lookups.
Use "none" as the base default and disable rmempty so LuCI always stores the selected resolver.
Maintainer
@stangri @egc112
Tested on
OpenWrt version: Snapshot
LuCI version: Latest