Skip to content

luci-app-ddns: replace dynamic_dns_lucihelper call on handleStopDDnsRule#8766

Open
feckert wants to merge 2 commits into
openwrt:masterfrom
TDT-AG:pr/20260701-ddns
Open

luci-app-ddns: replace dynamic_dns_lucihelper call on handleStopDDnsRule#8766
feckert wants to merge 2 commits into
openwrt:masterfrom
TDT-AG:pr/20260701-ddns

Conversation

@feckert

@feckert feckert commented Jul 1, 2026

Copy link
Copy Markdown
Member

Description

@champtar This should fix https://github.com/openwrt/packages/security/advisories/GHSA-hfpc-hw28-69pj

However, this change only works if the procd change is used to start the service.
This change is included in master and openwrt-25.12

Also, I'm no longer satisfied with the app. The app needs a little love. Please try it again with a different setup.

Checklist

  • [ x ] This PR is not from my main or master branch 💩, but a separate branch. ✅
  • [ x ] Each commit has a valid ✒️ Signed-off-by: <my@email.address> row (via git commit --signoff).
  • [ x ] Each commit and PR title has a valid 📝 <package name>: title first line subject for packages.

@github-actions

This comment has been minimized.

@champtar

champtar commented Jul 1, 2026

Copy link
Copy Markdown
Member

Thanks @feckert, on my phone so hard to review your change, I've pinged the initial reporter for review.

Also, I'm no longer satisfied with the app. The app needs a little love. Please try it again with a different setup.

You mean you want to stop maintaining it ? Or you have improvement ideas but no time ?

@champtar

champtar commented Jul 1, 2026

Copy link
Copy Markdown
Member

CI is not happy with the empty commit body

@feckert feckert force-pushed the pr/20260701-ddns branch from 96f86a9 to 348ea9d Compare July 1, 2026 10:11
@feckert

feckert commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

CI is not happy with the empty commit body

That should work now.

You mean you want to stop maintaining it ? Or you have improvement ideas but no time ?

Well, I didn't create the app. I just took it over, because I'm also the maintainer of the ddns-script package (which I'm not happy about either), but no one else is doing it.
The problem is simply time

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit. The core change (routing stop/reload through /etc/init.d/ddns per-section instead of dynamic_dns_lucihelper.sh, with the matching ACL update) is sound — the ddns init script uses USE_PROCD with per-section procd_open_instance "$cfg", so /etc/init.d/ddns stop <section> targets a single instance as intended. Two notes left inline on the reload.disabled change, which is functionally correct but diverges from the static render path and is outside the commit's stated scope.


Generated by Claude Code

Comment thread applications/luci-app-ddns/htdocs/luci-static/resources/view/ddns/overview.js Outdated
Comment thread applications/luci-app-ddns/htdocs/luci-static/resources/view/ddns/overview.js Outdated
feckert added 2 commits July 3, 2026 07:44
The ddns service is using procd for service execution. The
'dynamic_dns_lucihelper' ist not needed anymore.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
Since the switch to procd, there has been a separate service for each DDNS
provider. As a result, the logic behind the buttons for starting and
reloading the service has been incorrect since the switch.

This commit fixes the issue and standardizes the handling between the
polling and loading state of the overview page.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
@feckert feckert force-pushed the pr/20260701-ddns branch from 348ea9d to bf6c5d6 Compare July 3, 2026 06:52
@feckert

feckert commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

@champtar I rebased the button handling again based on a note from the AI CI/CD on the overview page. It actually wasn't working right. The whole thing was a bit tricky. Like I said, the app needs some love. Let's see if I can find the time.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit (bf6c5d6 "fix start/stop button logic"; cfedd55 is content-identical to the previously-reviewed revision). One inline finding on the reworked reload-button gating, which throws a TypeError for enabled-but-not-running sections in both the poll and static render paths.


Generated by Claude Code


reload.disabled = (status['_enabled'] == 0 || cfg_enabled == 0);
stop.disabled = (!service[section_id].pid);
reload.disabled = (cfg_enabled == 0) || (!service[section_id] && !service[section_id].pid);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(!service[section_id] && !service[section_id].pid) is broken in both directions. When service[section_id] is undefined — which happens for an enabled section whose service isn't currently running (the surrounding code reads service[section_id]?.ip with optional chaining, so undefined is expected here) — !service[section_id] is true and the && then evaluates !service[section_id].pid, throwing TypeError: Cannot read properties of undefined (reading 'pid') and aborting the poll refresh. Because such a section has cfg_enabled != 0, the || doesn't short-circuit, so the crash is reachable. When service[section_id] is truthy the term short-circuits to false, so the pid check is dead and reload ends up gated only on cfg_enabled.

The stop.disabled line directly below`` already uses the correct guard — mirror it:

Suggested change
reload.disabled = (cfg_enabled == 0) || (!service[section_id] && !service[section_id].pid);
reload.disabled = (cfg_enabled == 0) || !(service[section_id] && service[section_id].pid);

The same bug is in the static render path at line 558:`` (!resolved[section_id] && !resolved[section_id].pid) should likewise become `!(resolved[section_id] && resolved[section_id].pid)`.


Generated by Claude Code

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants