#8448 — [25.12] luci-app-adguardhome: add new app

openwrt/luci · View on GitHub · analyzed 2026-03-20
Author: GeorgeSapkin 25.12/add-luci-app-adguardhome → openwrt-25.12 Files changed: 6
## Summary

This PR adds a new LuCI application for AdGuard Home configuration to the OpenWrt 25.12 branch. The implementation is generally clean and follows LuCI conventions, but there are several correctness and security issues that need to be addressed before merging.

## Issues

**[critical]** `applications/luci-app-adguardhome/htdocs/luci-static/resources/view/adguardhome/config.js` (lines 23-24)

**What:** `RUNNING_SPAN` and `NOT_RUNNING_SPAN` are defined at module evaluation time by calling `_()` before the i18n system is initialized, so the strings will never be translated.
**Why it matters:** All users will always see the English strings "Running" / "Not running" regardless of their locale, silently breaking internationalization.
**Fix:** Convert these to functions or inline `_('Running')` / `_('Not running')` inside `getStatusValue()` where they are consumed at render time.

---

**[major]** `applications/luci-app-adguardhome/htdocs/luci-static/resources/view/adguardhome/config.js` (lines 211-220)

**What:** The sixth argument passed to `mainSect.taboption(...)` for the `gcOpt` field is a raw HTML anchor string (`'<a href="https://go.dev/doc/gc-guide#GOGC" …'`), but `form.Value`'s constructor only accepts five positional arguments (`section, option, title, description`) — there is no sixth slot.
**Why it matters:** The link is silently discarded and never rendered; the description also has a trailing comma before the stray argument, indicating a copy-paste mistake that went unnoticed.
**Fix:** Remove the stray sixth argument, and if the link is desired, embed it inside the description string (the fifth argument) after enabling `rawhtml` on the option.

---

**[major]** `applications/luci-app-adguardhome/htdocs/luci-static/resources/view/adguardhome/config.js` (lines 196-205)

**What:** `advSettingsOpt.load` reads from `sessionStorage` but returns either the stored string or `'0'`, however `form.Flag` expects the return to match its `enabled`/`disabled` values (`'1'`/`'0'`) and the field is never actually persisted to UCI — the tab visibility toggle based on `depends('advanced_settings', '1')` will therefore not survive a page reload correctly if the session value is stale or absent.
**Why it matters:** On a fresh page load the advanced settings fields governed by `.depends('advanced_settings', '1')` will always be hidden even if the user previously enabled them in the same session, because `sessionStorage.getItem` returns `null` (not `'0'`) when absent, and `null || '0'` gives `'0'`.  This is actually fine in that specific case, but `remove` is a no-op so if someone else saves the form the key remains in session storage indefinitely and could confuse state across tabs.
**Fix:** This is a minor logic concern, but document the intentional no-op `remove`/`write` overrides with a comment, and ensure `sessionStorage.getItem(STORAGE_KEY) ?? '0'` is used instead of `||` to be explicit about the null case.

---

**[major]** `applications/luci-app-adguardhome/htdocs/luci-static/resources/view/adguardhome/config.js` (line 18)

**What:** `PATH_REGEX = /^\/etc(\/[^/]+)?\/?\$$/` only blocks paths exactly one directory deep under `/etc` (e.g. `/etc/adguardhome`) but allows `/etc/adguardhome/sub/dir` — a user could store the config directly in `/etc/adguardhome/sub` and bypass the intent of "must be in its own directory."
**Why it matters:** The validation message says the file must be in its own directory, but the regex only rejects paths at depth 1 under `/etc`, not paths like `/etc/foo/bar/adguardhome.yaml` where the parent is still a shared directory.
**Fix:** Re-examine what the regex is actually meant to enforce and document it clearly; if the goal is simply "not directly in `/etc`", the regex is already correct and the description should be updated, otherwise tighten the regex.

---

**[minor]** `applications/luci-app-adguardhome/htdocs/luci-static/resources/view/adguardhome/config.js` (lines 253-257)

**What:** `poll.add(updateStatus(statusNode), POLL_INTERVAL)` is called unconditionally after `map.render()` but there is no corresponding `poll.remove()` when the view is destroyed (no `destroy()` lifecycle hook implemented).
**Why it matters:** Navigating away from and back to this view will add a new polling callback each time, causing multiple redundant RPC calls and potential memory/performance issues over a long session.
**Fix:** Implement a `destroy()` method on the view that calls `poll.remove(…)` with a saved reference to the poll callback, following the pattern used in other LuCI apps.

---

**[minor]** `applications/luci-app-adguardhome/htdocs/luci-static/resources/view/adguardhome/config.js` (lines 44-54)

**What:** `getVersion()` calls `fs.exec('/usr/bin/AdGuardHome', ['--version'])` but the ACL in `luci-app-adguardhome.json` grants exec permission for `"/usr/bin/AdGuardHome --version"` (the full command string as a key) which is the correct rpcd syntax, however if the binary path or arguments ever diverge between the JS and the ACL the call will silently fail and return `'unknown version'` without any user-visible indication.
**Why it matters:** Silently returning `'unknown version'` makes it impossible to distinguish between "not installed", "wrong path", and "permission denied" from the UI.
**Fix:** Check `res.code` or differentiate the error path, and display a more descriptive message when the binary is not found vs. a permission error.

---

**[minor]** `applications/luci-app-adguardhome/Makefile` (line 10)

**What:** `LUCI_EXTRA_DEPENDS` declares a minimum version constraint `adguardhome (>=0.107.73-r3)` but `LUCI_DEPENDS` already lists `+adguardhome` without a version constraint, meaning `LUCI_DEPENDS` will pull in any version of `adguardhome` regardless of the extra constraint.
**Why it matters:** On a system where `adguardhome` is present but older than `0.107.73-r3`, the package will install without error but may not function correctly.
**Fix:** Either consolidate to a single versioned dependency in `LUCI_DEPENDS` (e.g. `+adguardhome:adguardhome (>=0.107.73-r3)`) or verify this is the intended LuCI build-system behavior for `LUCI_EXTRA_DEPENDS` and add a comment explaining the distinction.

## Suggestions

- The `'require poll'` and `'require dom'` requires at the top are used, but consider adding `'require uci'` if future options need it — or remove the unused `STORAGE_KEY`-related session-storage approach and use a proper in-memory variable on the view object (`this._showAdvanced`) to avoid touching browser storage for a transient UI state.
- `validateConfigFile` returns `true` for empty/null values with a comment that defaults are used; consider adding a comment explaining this is intentional (empty = use default) so future maintainers don't accidentally add a required-field check.
- The `getServiceInfo` factory function creates a new `rpc.declare` closure per call but `getAGHServiceInfo` is only called in one place; simplifying to a direct `rpc.declare` at module scope would be cleaner and easier to follow.
- Consider adding an "Apply & Restart" button or at minimum a note in the UI that UCI changes require a service restart to take effect, since AdGuard Home's init script reads UCI on start.
- The `.pot` file has no `PO-Revision-Date`, `Language`, `MIME-Version`, or other standard gettext headers beyond `Content-Type`; this may cause issues with some gettext tooling.

## Verdict

**Request Changes** — The stray sixth argument to `taboption` (silently dropped link), the `_()` call at module-evaluation time breaking all translations, and the missing poll cleanup are correctness issues that must be fixed before this can be merged.