Merge pull request #1871 from sebingel/fix/piholeapi-verify-ssl

PIHOLEAPI: make the Verify SSL setting work and check setting keys

docs/PIHOLE_GUIDE.md

@@ -15,7 +15,7 @@ To use this approach, make sure a Web UI password is configured in **Pi-hole**.
 | `PIHOLEAPI_URL`            | Your Pi-hole base URL, including the port.                                                       | `http://192.168.1.82:9880/` |
 | `PIHOLEAPI_RUN_SCHD`       | If you run multiple device scanner plugins, configure them to use the same schedule.             | `*/5 * * * *`               |
 | `PIHOLEAPI_PASSWORD`       | The Pi-hole Web UI admin password. NetAlertX automatically handles Base64 encoding and decoding. | `passw0rd`                  |
-| `PIHOLEAPI_SSL_VERIFY`     | Whether to verify HTTPS certificates. Disable only when using self-signed certificates.          | `False`                     |
+| `PIHOLEAPI_VERIFY_SSL`     | Whether to verify HTTPS certificates. Disable only when using self-signed certificates.          | `False`                     |
 | `PIHOLEAPI_API_MAXCLIENTS` | Maximum number of devices to request from Pi-hole. The default value is usually sufficient.      | `500`                       |
 | `PIHOLEAPI_FAKE_MAC`       | Generate a deterministic fake MAC address from the IP address.                                   | `False`                     |
 

docs/PLUGINS_DEV.md

@@ -246,6 +246,7 @@ Check your plugin against these repo-wide conventions before opening a PR (verif
 - **Persist plugin state under `dbFolderPath`, config artifacts under `configPath`** — see [Persisting Plugin Data](#persisting-plugin-data-state--config-files) below.
 - **A plugin mapped to `mapped_to_table: "CurrentScan"` must also map `scanSourcePlugin`** (a static value via `mapped_to_column_data`, see [Static Value Mapping](#static-value-mapping) below), **and that value must equal the plugin's own `unique_prefix` exactly.** Whether the mapping is present at all isn't mechanically enforced by `test_plugin_conventions.py`, so review that by eye - but `test_scan_source_plugin_matches_unique_prefix` does mechanically check the value matches `unique_prefix` whenever the mapping exists, since `update_devices_data_from_scan()` (`server/scan/device_handling.py`) reads it verbatim to build the `<value>_SET_ALWAYS`/`<value>_SET_EMPTY` settings keys (`server/db/authoritative_handler.py`) - any mismatch (wrong case, a friendly display name, punctuation) means those settings silently never resolve, so the plugin's SET_ALWAYS/SET_EMPTY overrides get quietly ignored (this shipped for real: `freebox` used `"Freebox"` against `unique_prefix: "FREEBOX"`, `fritzbox` used `"Fritz!Box"` against `"FRITZBOX"`). Omitting the mapping entirely leaves `scanSourcePlugin` `NULL` on every row this plugin inserts, which silently breaks two other things: `create_new_devices()`'s `plugin_prefix = str(scanSourcePlugin).strip() if scanSourcePlugin else "NEWDEV"` mislabels devices this plugin creates as source `NEWDEV`; and `update_devices_data_from_scan()`'s `SELECT DISTINCT scanSourcePlugin FROM CurrentScan` + `[row[0] for row in plugin_rows if row[0]] or [None]` drops the `NULL` rows entirely (the `or [None]` fallback never triggers once any other plugin contributes a non-null prefix), so this plugin's `CurrentScan` rows never get picked up by the per-plugin device-update loop at all - the plugin can *insert* into `CurrentScan` but never actually confirm/update a device's presence.
 - **A setting's `dataType` and `default_value` must actually agree.** `dataType: "array"` (or `"object"`) means `default_value` must be a real JSON literal for that shape — `'["default"]'`, not the bare string `"default"`. `setting_value_to_python_type()` (`server/helper.py`) `json.loads()`s the default at runtime; a bare string fails that parse, silently logs a decode error, and returns `[]` instead of your intended default — this shipped for real in `devParentRelType`/`UI_theme`/`UI_TOPOLOGY_ORDER` before being caught. If `elementOptions` already sets `multiple`/`orderable: "false"`, that's a strong signal the setting is actually scalar and `dataType` should be `"string"`, not `"array"`, regardless of what UI widget (`select`, etc.) renders it.
+- **Read a setting under the exact key `config.json` defines** (`<unique_prefix>_<function>`, e.g. `PIHOLEAPI_VERIFY_SSL` for the `VERIFY_SSL` setting of `PIHOLEAPI`). `get_setting_value()` returns `""` for an unknown key, so a misspelled key silently turns the setting off. `test_own_setting_keys_exist_in_config` (`test/plugins/test_plugin_conventions.py`) checks every literal key that starts with the plugin's own prefix.
 - **Only set `"allow_raw_text": true` on a display-only column type (`textarea_readonly`).** Every plugin-sourced field is HTML/control-char-stripped by default before it reaches the DB; see [Field Sanitization](#field-sanitization) below. `test_allow_raw_text_only_on_safe_types` (`test/plugins/test_plugin_conventions.py`) enforces the type restriction; it can't catch a column that legitimately needs the opt-out but is rendered somewhere unsafe, so use it only for values that are never interpreted as HTML.
 - **A new system package or Python dependency needs mirroring across every build target it should work on, not just the root `Dockerfile`.** `.devcontainer/Dockerfile` is auto-generated (its own header says so) by `.devcontainer/scripts/generate-configs.sh`, which concatenates the root `Dockerfile` with `.devcontainer/resources/devcontainer-Dockerfile`. It's a committed, git-tracked file, not something read fresh at build time - a package added to the root `Dockerfile` without re-running that script leaves the devcontainer's checked-in Dockerfile stale, so the package is silently missing there until someone regenerates it. `Dockerfile.debian` (`docs/BUILDS.md`) is a second, separately-maintained build target with its own `apt-get install` package list and `setcap` calls - a system package (and its `setcap` grant, if it needs one) has to be added there too, not just to the Alpine `Dockerfile`. For a new Python dependency, the root `requirements.txt` is not the only one either: `install/ubuntu24/requirements.txt` and `install/proxmox/requirements.txt` are separate lists for their respective non-Docker install methods, not mechanically kept in sync with the root file (no CI check covers this) - check whether the new dependency is actually needed by those install methods too rather than assuming the root file alone is enough.
 

server/plugins/pihole_api_scan/README.md

@@ -31,7 +31,7 @@ No additional Pi-hole configuration is required.
 | ---------------------------- | -------------------------------------------------------------------------------- |
 | **PIHOLEAPI_URL**            | Your Pi-hole base URL.                                                           |
 | **PIHOLEAPI_PASSWORD**       | The Web UI base64 encoded (en-/decoding handled by the app) admin password.      |
-| **PIHOLEAPI_SSL_VERIFY**     | Whether to verify HTTPS certificates. Disable only for self-signed certificates. |
+| **PIHOLEAPI_VERIFY_SSL**     | Whether to verify HTTPS certificates. Disable only for self-signed certificates. |
 | **PIHOLEAPI_RUN_TIMEOUT**    | Request timeout in seconds.                                                      |
 | **PIHOLEAPI_API_MAXCLIENTS** | Maximum number of devices to request from Pi-hole. Defaults are usually fine.    |
 | **PIHOLEAPI_FAKE_MAC**       | Generate FAKE MAC from IP.                                                       |

server/plugins/pihole_api_scan/pihole_api_scan.py

@@ -75,6 +75,11 @@ def pihole_api_auth():
     except requests.exceptions.Timeout:
         mylog('none', [f'[{pluginName}] Pi-hole auth request timed out. Try increasing PIHOLEAPI_RUN_TIMEOUT.'])
         return False
+    except requests.exceptions.SSLError as e:
+        # SSLError is a subclass of ConnectionError, so it must be caught first
+        mylog('none', [f'[{pluginName}] TLS certificate check failed during Pi-hole auth: {e}. '
+                       'Turn off PIHOLEAPI_VERIFY_SSL if Pi-hole uses a self-signed certificate.'])
+        return False
     except requests.exceptions.ConnectionError:
         mylog('none', [f'[{pluginName}] Connection error during Pi-hole auth. Check PIHOLEAPI_URL and PIHOLEAPI_PASSWORD'])
         return False
@@ -281,7 +286,7 @@ def main():
     PIHOLEAPI_PASSWORD = get_setting_value('PIHOLEAPI_PASSWORD')
     PIHOLEAPI_API_MAXCLIENTS = get_setting_value('PIHOLEAPI_API_MAXCLIENTS')
     # Accept boolean or string "True"/"False"
-    PIHOLEAPI_VERIFY_SSL = get_setting_value('PIHOLEAPI_SSL_VERIFY')
+    PIHOLEAPI_VERIFY_SSL = get_setting_value('PIHOLEAPI_VERIFY_SSL')
     PIHOLEAPI_RUN_TIMEOUT = get_setting_value('PIHOLEAPI_RUN_TIMEOUT')
     PIHOLEAPI_GET_OFFLINE = get_setting_value('PIHOLEAPI_GET_OFFLINE')
     PIHOLEAPI_CONSIDER_ONLINE = get_setting_value('PIHOLEAPI_CONSIDER_ONLINE')

test/plugins/test_pihole_api_scan_verify_ssl.py

@@ -0,0 +1,117 @@
+"""Tests for the "Verify SSL" setting of the pihole_api_scan (PIHOLEAPI) plugin.
+
+pihole_api_scan.py is loaded with its NetAlertX-internal dependencies stubbed
+out (same approach as test_adguard_import.py), so these tests run without a
+Pi-hole instance. Only the HTTP call to Pi-hole is mocked; main() and
+pihole_api_auth() run for real.
+"""
+
+import importlib.util
+import os
+import sys
+import types
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+
+INSTALL_PATH = os.getenv("NETALERTX_APP", "/app")
+sys.path.extend([f"{INSTALL_PATH}/server/plugins", f"{INSTALL_PATH}/server"])
+
+
+def _load_pihole_api_scan_module():
+    """Import pihole_api_scan.py with NetAlertX modules stubbed, then restore sys.modules."""
+    missing_module = object()
+    previous_modules = {}
+
+    def stub(name, **attributes):
+        """Put a fake module with the given attributes into sys.modules, remembering the previous entry."""
+        previous_modules[name] = sys.modules.get(name, missing_module)
+        module = types.ModuleType(name)
+        for attribute, value in attributes.items():
+            setattr(module, attribute, value)
+        sys.modules[name] = module
+
+    stub("plugin_helper", Plugin_Objects=MagicMock, is_mac=MagicMock(return_value=True))
+    stub("utils.datetime_utils", timeNowUTC=MagicMock())
+    stub("logger", mylog=MagicMock(), Logger=MagicMock())
+    stub("helper", get_setting_value=MagicMock(return_value="UTC"))
+    stub("const", logPath="/tmp")
+    stub("conf", tz=None)
+    stub("pytz", timezone=MagicMock(return_value="UTC"))
+    stub("utils.crypto_utils", string_to_fake_mac=MagicMock(return_value="fa:ce:00:00:00:01"))
+
+    module_path = Path(__file__).resolve().parents[2] / "server" / "plugins" / "pihole_api_scan" / "pihole_api_scan.py"
+    spec = importlib.util.spec_from_file_location("pihole_api_scan_verify_ssl_script", module_path)
+    module = importlib.util.module_from_spec(spec)
+    try:
+        spec.loader.exec_module(module)
+    finally:
+        for name, previous_module in previous_modules.items():
+            if previous_module is missing_module:
+                sys.modules.pop(name, None)
+            else:
+                sys.modules[name] = previous_module
+
+    return module
+
+
+pihole_api_scan = _load_pihole_api_scan_module()
+
+
+def _use_settings(monkeypatch, settings):
+    """Serve settings to main() via get_setting_value, and restore the module globals main() rebinds after the test."""
+    # raising=False: a global may only come into existence when main() runs.
+    for name in ("PIHOLEAPI_URL", "PIHOLEAPI_PASSWORD", "PIHOLEAPI_API_MAXCLIENTS", "PIHOLEAPI_VERIFY_SSL",
+                 "PIHOLEAPI_RUN_TIMEOUT", "PIHOLEAPI_GET_OFFLINE", "PIHOLEAPI_CONSIDER_ONLINE"):
+        monkeypatch.setattr(pihole_api_scan, name, getattr(pihole_api_scan, name, None), raising=False)
+    monkeypatch.setattr(pihole_api_scan, "get_setting_value", settings.get)
+
+
+def _login_failure(monkeypatch, error):
+    """Run pihole_api_auth() with the login request raising error; return its result and everything it logged as one string."""
+    log = MagicMock()
+    monkeypatch.setattr(pihole_api_scan, "mylog", log)
+    monkeypatch.setattr(pihole_api_scan, "PIHOLEAPI_URL", "https://pi.hole/")
+    monkeypatch.setattr(pihole_api_scan, "PIHOLEAPI_VERIFY_SSL", True)
+    monkeypatch.setattr(pihole_api_scan.requests, "post", MagicMock(side_effect=error))
+
+    result = pihole_api_scan.pihole_api_auth()
+    return result, " ".join(str(call.args[1]) for call in log.call_args_list)
+
+
+class TestVerifySsl:
+    def test_certificate_error_points_to_verify_ssl_setting(self, monkeypatch):
+        """A failed TLS certificate check at login names PIHOLEAPI_VERIFY_SSL instead of sending the user to URL and password."""
+        result, logged = _login_failure(monkeypatch, pihole_api_scan.requests.exceptions.SSLError("certificate verify failed"))
+
+        assert result is False
+        assert "PIHOLEAPI_VERIFY_SSL" in logged
+        assert "Check PIHOLEAPI_URL and PIHOLEAPI_PASSWORD" not in logged
+
+    def test_plain_connection_error_still_points_to_url_and_password(self, monkeypatch):
+        """A connection error that is not a certificate problem keeps pointing to PIHOLEAPI_URL and PIHOLEAPI_PASSWORD."""
+        result, logged = _login_failure(monkeypatch, pihole_api_scan.requests.exceptions.ConnectionError("connection refused"))
+
+        assert result is False
+        assert "Check PIHOLEAPI_URL and PIHOLEAPI_PASSWORD" in logged
+        assert "PIHOLEAPI_VERIFY_SSL" not in logged
+
+    @pytest.mark.parametrize("verify_ssl", [True, False])
+    def test_verify_ssl_setting_reaches_pihole_request(self, monkeypatch, verify_ssl):
+        """The VERIFY_SSL setting (key PIHOLEAPI_VERIFY_SSL) is what main() passes as verify= to the Pi-hole API."""
+        _use_settings(monkeypatch, {
+            "PIHOLEAPI_URL": "https://pi.hole/",
+            "PIHOLEAPI_PASSWORD": "secret",
+            "PIHOLEAPI_RUN_TIMEOUT": 30,
+            "PIHOLEAPI_VERIFY_SSL": verify_ssl,
+        })
+        auth_response = MagicMock()
+        auth_response.json.return_value = {"session": {"valid": False}}
+        post = MagicMock(return_value=auth_response)
+        monkeypatch.setattr(pihole_api_scan.requests, "post", post)
+        monkeypatch.setattr(pihole_api_scan.requests.packages.urllib3, "disable_warnings", MagicMock())
+
+        pihole_api_scan.main()
+
+        assert post.call_args.kwargs["verify"] is verify_ssl

test/plugins/test_plugin_conventions.py

@@ -235,6 +235,38 @@ def test_hardcoded_default_matches_config(plugin_name):
     )
 
 
+# ---------------------------------------------------------------------------
+# A plugin reading one of its own settings under a key config.json never defines
+#
+# get_setting_value() returns "" for a key nobody defined, so a misspelled key
+# silently turns the setting off. Only literal keys that start with the
+# plugin's own unique_prefix are checked; core settings and other plugins'
+# keys are out of scope.
+# ---------------------------------------------------------------------------
+_SETTING_KEY_RE = re.compile(r'''get_setting_value\(\s*["']([A-Za-z0-9_]+)["']''')
+
+
+@pytest.mark.parametrize('plugin_name', _PLUGIN_NAMES)
+def test_own_setting_keys_exist_in_config(plugin_name):
+    """Every literal get_setting_value('<PREFIX>_X') in a plugin's scripts names a setting its config.json defines."""
+    config = _load_config(plugin_name)
+    own_prefix = config.get('unique_prefix', '') + '_'
+    defined = set(_setting_defaults(config))
+
+    unknown = []
+    for py_file in _plugin_py_files(plugin_name):
+        with open(py_file) as f:
+            source = f.read()
+        for key in _SETTING_KEY_RE.findall(source):
+            if key.startswith(own_prefix) and key not in defined:
+                unknown.append(f"{os.path.basename(py_file)}: get_setting_value('{key}')")
+
+    assert not unknown, (
+        f"{plugin_name}: setting key(s) not defined in config.json - get_setting_value() returns '' for them, "
+        "so the setting is silently ignored:\n" + "\n".join(unknown)
+    )
+
+
 # ---------------------------------------------------------------------------
 # RUN_TIMEOUT reused as a per-call timeout inside a loop
 #