Addresses CodeRabbit review on PR #1765 (pullrequestreview-5069337680).
pihole_monitor.py:
- last_raw is now tracked per source ({"primary": N, "secondary": M}
per device) instead of one combined value. Combining raw totals
across sources before diffing let a counter reset on one instance
silently net out against real traffic on the other - e.g. primary
+2000 (a real spike) and secondary resetting 1000->5 (-995) would
combine into a raw delta of only 1005, hiding most of the primary's
actual spike behind the secondary's unrelated restart.
- New aggregate_source_deltas(): diffs each source independently via
compute_delta(), then sums only the valid deltas. A source with no
valid delta this run (bootstrapping or just reset) contributes
nothing and doesn't block the others; each source keeps its own
reference point going forward.
- State loaded from before this change (last_raw as a plain number,
not per-source) is now tolerated instead of crashing - treated as no
prior reference point, so every source just bootstraps fresh on the
next run.
README.md:
- Fixed a self-contradicting line: a less frequent schedule means
larger per-run deltas, so PIHOLEMON_MIN_BLOCKED may need *raising*,
not lowering as it previously said.
- Corrected PIHOLEMON_HISTORY_DAYS guidance: it's a retention window,
not a detection delay. A new device becomes evaluable on its 3rd
successful run (1st anchors the counter, 2nd records the first
delta, 3rd has a baseline to compare against), not after the full
retention window.
Tests: 54 (up from 48). New coverage: aggregate_source_deltas() unit
tests including the exact dual-source reset-masking scenario, a
main()-level integration test for the same, and a regression test for
tolerating pre-per-source state. Both the reset-masking fix and the
legacy-state guard verified via mutation testing (reverted each,
confirmed the relevant tests fail, restored). 99% line+branch coverage
maintained.
References PR #1765.
Docs:
- Added PIHOLEMON to docs/PLUGINS.md and a new "Approach 4" section in
docs/PIHOLE_GUIDE.md, leading with anomaly detection (the actual
differentiator vs PIHOLEAPI) and explaining when to pick each plugin.
- README/PLUGINS.md/config.json's UI-facing description all reordered
and shortened to lead with anomaly detection instead of device
import, and to drop implementation detail that belongs in the
README, not the Settings page.
- Trimmed the "Why not extend PIHOLEAPI" README section per feedback -
useful context for a maintainer, not for an end user configuring
the plugin.
config.json / pihole_monitor.py:
- RUN defaults to "disabled", matching every other non-core plugin.
- VERIFY_SSL split into PRIMARY_VERIFY_SSL / SECONDARY_VERIFY_SSL -
each instance can be http/https independently. Settings reordered so
each *_VERIFY_SSL sits right under its matching *_PASSWORD.
- GRAPHQL_TOKEN removed; graphql_token now reads the core API_TOKEN
setting instead of a plugin-specific duplicate.
- GRAPHQL_URL replaced with a GET_OWNER boolean - the endpoint is now
derived from this app's own GRAPHQL_PORT (single source of truth)
instead of a URL the user had to keep in sync by hand.
- HISTORY_LENGTH (run count) replaced with HISTORY_DAYS (a real time
window): state now stores [timestamp, delta] samples and
trim_history() drops anything older than the window, so the
baseline means the same thing regardless of schedule - a faster
schedule adds more data points instead of shrinking the window.
- STATE_FILE moved from the log folder to dbFolderPath, so the rolling
anomaly baseline survives NetAlertX upgrades instead of being wiped
with the logs.
- netalertx_device_owner() (1 GraphQL call per device) replaced by
netalertx_device_owners() (1 call per run, batched) - avoids N
blocking round-trips on a large network.
- Fixed a zero-baseline bug: `bool(... and baseline and ...)` silently
exempted a device with an all-zero blocked-query history (0.0 is
falsy in Python) from ever being flagged, even on its first real
spike. Now checks `baseline is not None`.
- Fixed the placeholder-MAC filter: only excluded the literal "ip-::",
not Pi-hole's general "ip-<address>" placeholder pattern. Caught
downstream by is_mac() either way, but now the actual placeholder
check does what it looks like it does.
- Fixed a cumulative-counter bug: Pi-hole's /api/stats/top_clients
returns a count that's cumulative since FTL last started, not a
per-interval or daily-resetting one (confirmed against FTL's own
source and long-standing user reports that it doesn't reset at
midnight). Comparing that raw total directly against a rolling
average made any device's ordinary growing traffic look like an
escalating anomaly. compute_delta() now diffs each run's raw count
against the previous run's (state gained a per-key last_raw
reference point alongside the delta history) - None (not 0) on the
first-ever run for a device or right after a counter reset, so
those runs re-anchor the reference point instead of fabricating or
swallowing a delta.
- RUN_SCHD default changed from every 6 hours to every 5 minutes now
that the baseline window is real days, not run count, so a frequent
schedule only adds data points instead of narrowing the window; also
matches the default most other device-scanner plugins use.
- RUN_SCHD gained the same live cron-validity checkmark ARPSCAN and
other scanner plugins use (a ✓/✗ icon next to the field, validated
client-side against a regex) - reuses the existing generic
validateRegex() widget, nothing plugin-specific to build.
Tests: 48 tests (up from 37), 99% line+branch coverage. Every fix
above verified via mutation testing (deliberately broken, confirmed
the relevant test fails, then restored).
CodeRabbit follow-up on PR #1765
(https://github.com/netalertx/NetAlertX/pull/1765#discussion_r3888374014):
test_main_history_length_never_produces_empty_or_growing_unbounded only
asserted len(history) >= 1, which a mis-clamped history_length (e.g.
keeping 4 items instead of 1) would still pass unnoticed.
Replaced with test_main_history_length_clamps_and_trims_exactly,
seeding distinct ordered values and asserting the exact retained
history against each PIHOLEMON_HISTORY_LENGTH boundary. Verified it
actually catches a broken clamp: temporarily reverted the
max(1, ...) fix in pihole_monitor.py, confirmed this test fails
([] == [40]) while the rest of the suite still passes, then restored
the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CHJAArRiet4GmXUsxnNLdW
Addresses 5 of the 6 actionable comments from CodeRabbit's review of
PR #1765 (netalertx/NetAlertX#1765), plus adds test coverage:
- fetch_top_blocked_clients() returns None on failure instead of {},
so a failed request can no longer be mistaken for "genuinely zero
blocked queries this run" and silently write a false 0 into a
device's rolling history baseline. main() now tracks a
stats_complete flag and skips anomaly evaluation + history
persistence entirely for a run with incomplete blocked-query data.
- fetch_top_blocked_clients() is now called with count=max_clients
(the existing PIHOLEMON_API_MAXCLIENTS setting) instead of a
hardcoded default of 50, so clients beyond the top 50 are no longer
silently dropped from anomaly detection.
- New build_ip_to_mac() derives the IP->MAC identity map from every
gathered device entry instead of from merge_device_entries()'s
by-MAC-deduplicated output, which only kept one IP per device and
silently lost a multi-IP device's other IPs (misattributing their
blocked-query traffic to a bare IP instead of the real MAC).
- PIHOLEMON_HISTORY_LENGTH is clamped to at least 1, so a negative
setting can no longer reach the history[-history_length:] slice
with a nonsensical negative-of-negative length.
- PIHOLEMON_VERIFY_SSL now defaults to true (was false, matching the
official PIHOLEAPI plugin's convention). README documents the
http:// vs https:// credentials trade-off explicitly rather than
forcing https:// - most home Pi-hole setups, including the one this
plugin targets, run over plain HTTP on a trusted LAN.
- Added test/plugins/test_pihole_monitor.py (37 tests, 99% line and
branch coverage of pihole_monitor.py per pytest-cov - only the
`if __name__ == '__main__':` entry-point guard is unreached):
auth and deauth success/failure paths, the None-sentinel-on-failure
contract, fetch_devices()'s own failure path, build_ip_to_mac()'s
multi-IP fix, gather_device_entries()'s skip branches and fake-MAC
fallback, netalertx_device_owner()'s success/failure/no-URL paths,
and main()-level coverage for source aggregation, the
stats_complete gate, the history_length boundary clamp, the
CONSIDER_ONLINE fallback, an unconfigured-sources run, and the
offline-device / invalid-MAC / unknown-IP / owner-lookup branches
together in one run.
Not addressed: CodeRabbit's suggestion to hard-reject http:// URLs in
auth(). Diverges deliberately - it would break the plugin's majority
use case (Pi-hole admin API on a trusted home LAN without TLS), which
this repo's own PIHOLEAPI plugin also targets over plain HTTP.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CHJAArRiet4GmXUsxnNLdW
A custom header carrying a non-ASCII character raised UnicodeEncodeError from
inside http.client. That is a ValueError, not a RequestException, so it escaped
both handlers in send() and took the whole publisher down - every notification
lost because of one typo in one header.
build_custom_headers now rejects newlines and non-ASCII the same way it already
rejects malformed and colliding entries: warn, skip that entry, keep the rest.
UnicodeEncodeError is still caught at the request, as a backstop for the
plugin's own headers, since REPORT_DASHBOARD_URL feeds one of them.
Verified against real requests: before, send() raised UnicodeEncodeError; after,
the bad header is dropped and the notification is still posted.
The custom header added in #1695 was a single name/value pair, which is
enough for a proxy that authenticates with one token but not for Pangolin,
which expects both P-Access-Token-Id and P-Access-Token.
NTFY_CUSTOMHEADER_NAME and NTFY_CUSTOMHEADER_VALUE are replaced by a single
list setting, NTFY_CUSTOM_HEADERS, holding one "Name: Value" entry per
header. The list widget is the same one the other list settings use.
Only the first colon separates the name from the value, so values may
contain colons. An entry is skipped and logged when it is malformed, when
the name repeats, or when it collides with a header the plugin already set,
so a custom header still cannot clobber the ntfy credentials.
Values are never written to the log, since they are usually secrets. That
also applies to the invalid-header error, which now names the headers that
were applied without quoting any of them.
- Introduced a new plugin for monitoring website health, including functionality to check URLs and log results.
- Created README and configuration files for the workflows plugin, detailing its purpose and settings.
- Updated import paths in various test files to reflect the new directory structure.
- Ensured compatibility of test cases with the updated plugin architecture.
- Implemented the REST Import plugin (rest_import.py) to handle importing data from REST APIs.
- Added functionality for configurable HTTP methods, authentication types, and custom headers.
- Included error handling for various HTTP response statuses and connection issues.
- Created unit tests for the plugin covering header building, path resolution, MAC validation, record mapping, and authentication methods.
- Ensured that module-level side effects are patched during tests to prevent live interactions.
Replaces the raw sqlite3 query in get_netalertx_devices() with
DeviceInstance().getAll() as suggested in code review, applying the
archived/offline/new filters in Python. Removes the sqlite3 and
fullDbPath imports. Updates tests to mock DeviceInstance.getAll().
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- config.json: add show:true to all visible column definitions so they
render in the plugin output table
- script.py: fix managed_names adoption bug — update/skip branches no
longer add unowned clients to managed state; rename tracking now
scoped to plugin-created clients only
- README.md: fix ADGUARDEXP_URL default (localhost:3000, not local IP),
add language tags to fenced code blocks, normalise metadata block to
Other info / Maintainer / DD-Mon-YYYY format
- test_adguard_export.py: add regression test for manual client matched
by ID not being adopted into managed state
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a new NetAlertX plugin that syncs known devices from the NetAlertX
database to AdGuard Home as persistent clients, keeping names, MACs, IP
addresses, and device-type tags in sync.
Also fixes the adguard_import config.json description placeholder and a
minor indentation inconsistency in that file.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two independent reliability problems were identified during PR readiness
review. First, FritzConnection had no explicit timeout, meaning an
unreachable or slow Fritz!Box would block the plugin process indefinitely
until the OS TCP timeout fired (typically 2+ minutes), making the 60s
RUN_TIMEOUT in config.json ineffective. Second, hashlib.md5() called
without usedforsecurity=False raises ValueError on FIPS-enforced systems
(common in enterprise Docker hosts), silently breaking the guest WiFi
synthetic device feature for those users.
Changes:
- Add timeout=10 to FritzConnection(...) call (fritzbox.py:57)
The fritzconnection library accepts a timeout parameter directly in
__init__; it applies per individual HTTP request to the Fritz!Box,
bounding each TR-064 call including the initial connection handshake.
- Add usedforsecurity=False to hashlib.md5() call (fritzbox.py:191)
The MD5 hash is used only for deterministic MAC derivation (not for
any security purpose), so the flag is semantically correct and lifts
the FIPS restriction without changing the computed value.
- Update test assertion to include timeout=10 (test_fritzbox.py:307)
assert_called_once_with checks the exact call signature; the test
expectation must match the updated production code.
The plugin now fails fast on unreachable Fritz!Box (within 10s per
request) and works correctly on FIPS-enabled hosts. Default behavior
for standard deployments is unchanged.
NetAlertX had no native support for discovering devices connected to
Fritz!Box routers. Users relying on Fritz!Box as their primary home
router had to use generic network scanning (ARP/ICMP), missing
Fritz!Box-specific details like interface type (WiFi/LAN) and
connection status per device.
Changes:
- Add plugin implementation (front/plugins/fritzbox/fritzbox.py)
Queries all hosts via FritzHosts TR-064 service, normalizes MACs,
maps interface types (802.11→WiFi, Ethernet→LAN), and writes results
to CurrentScan via Plugin_Objects. Supports filtering to active-only
devices and optional guest WiFi monitoring via a synthetic AP device
with a deterministic locally-administered MAC (02:xx derived from
Fritz!Box MAC via MD5).
- Add plugin configuration (front/plugins/fritzbox/config.json)
Defines plugin_type "device_scanner" with settings for host, port,
credentials, guest WiFi reporting, and active-only filtering.
Maps scan columns to CurrentScan fields (scanMac, scanLastIP, scanName,
scanType). Default schedule: every 5 minutes.
- Add plugin documentation (front/plugins/fritzbox/README.md)
Covers TR-064 protocol basics, quick setup guide, all settings with
defaults, troubleshooting for common issues (connection refused, auth
failures, no devices found), and technical details.
- Add fritzconnection>=1.15.1 dependency (requirements.txt)
Required Python library for TR-064 communication with Fritz!Box.
- Add test suite (test/plugins/test_fritzbox.py:1-298)
298 lines covering get_connected_devices (active filtering, MAC
normalization, interface mapping, error resilience), check_guest_wifi_status
(service detection, SSID-based guest detection, fallback behavior), and
create_guest_wifi_device (deterministic MAC generation, locally-administered
bit, fallback MAC, regression anchor with precomputed hash).
Users can now scan Fritz!Box-connected devices natively, seeing per-device
connection status and interface type directly in NetAlertX. Guest WiFi
monitoring provides visibility into guest network state. The plugin
defaults to HTTPS on port 49443 with active-only filtering enabled.
jokob-sk