diff --git a/docs/PIHOLE_GUIDE.md b/docs/PIHOLE_GUIDE.md index ecfcda6a1..c3302e851 100755 --- a/docs/PIHOLE_GUIDE.md +++ b/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` | diff --git a/docs/PLUGINS_DEV.md b/docs/PLUGINS_DEV.md index cfce62b93..eeca8dda8 100755 --- a/docs/PLUGINS_DEV.md +++ b/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 `_SET_ALWAYS`/`_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** (`_`, 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. diff --git a/server/plugins/pihole_api_scan/README.md b/server/plugins/pihole_api_scan/README.md index df6f92cef..c7fd655ff 100644 --- a/server/plugins/pihole_api_scan/README.md +++ b/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. | diff --git a/server/plugins/pihole_api_scan/pihole_api_scan.py b/server/plugins/pihole_api_scan/pihole_api_scan.py index 2afcd6df6..59287be82 100644 --- a/server/plugins/pihole_api_scan/pihole_api_scan.py +++ b/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') diff --git a/test/plugins/test_pihole_api_scan_verify_ssl.py b/test/plugins/test_pihole_api_scan_verify_ssl.py new file mode 100644 index 000000000..c4d3898c7 --- /dev/null +++ b/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 diff --git a/test/plugins/test_plugin_conventions.py b/test/plugins/test_plugin_conventions.py index 939e6deab..1753ee77e 100644 --- a/test/plugins/test_plugin_conventions.py +++ b/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('_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 #