Sync - #1769
Merged
Merged
Conversation
FE: Implement pause/resume functionality for automatic scans with API…
Currently translated at 100.0% (832 of 832 strings) Translation: NetAlertX/core Translate-URL: https://hosted.weblate.org/projects/pialert/core/it/
Currently translated at 88.1% (737 of 836 strings) Translation: NetAlertX/core Translate-URL: https://hosted.weblate.org/projects/pialert/core/de/
Currently translated at 100.0% (836 of 836 strings) Translation: NetAlertX/core Translate-URL: https://hosted.weblate.org/projects/pialert/core/fr/
Currently translated at 100.0% (836 of 836 strings) Translation: NetAlertX/core Translate-URL: https://hosted.weblate.org/projects/pialert/core/it/
fix(mikrotik): continue scans past leases without MAC addresses
Currently translated at 100.0% (836 of 836 strings) Translation: NetAlertX/core Translate-URL: https://hosted.weblate.org/projects/pialert/core/zh_Hans/
Currently translated at 100.0% (836 of 836 strings) Translation: NetAlertX/core Translate-URL: https://hosted.weblate.org/projects/pialert/core/zh_Hans/
Signed-off-by: Vaibhav Srivastava <vaibhavsri1712@gmail.com>
docs: fix typo shoudl -> should
…maly detection Does two jobs against the same Pi-hole connection(s), instead of two separately configured plugins: 1. Device import - same job as the official PIHOLEAPI (pihole_api_scan) plugin, but supports an optional secondary/failover Pi-hole natively (accepts two sets of credentials instead of forking the official plugin, which hardcodes its settings-key prefix and doesn't support multiple instances). 2. Query anomaly detection - flags a device whose blocked-query count spikes well above its own recent rolling average (signature of malware/a compromised device beaconing out), keyed by MAC address (not IP, which changes under DHCP) and combined across both Pi-hole instances so a compromised device can't evade detection by switching resolvers. Notifications are delegated entirely to NetAlertX's own Watched/Report on mechanism - the plugin never calls a notification service directly. Live-tested against a two-Pi-hole home setup (v26.8.5) for several days, including two real bugs found and fixed during that testing (an offline-filtered device losing its MAC and falling back to a bare-IP identifier, and a boolean-expression flake8 style fix).
Updated download link to point to new netalertx repository instead of older jokob-sk repository URL.
Update download link for Proxmox installer
Addresses 5 of the 6 actionable comments from CodeRabbit's review of PR #1765 (#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
CodeRabbit follow-up on PR #1765 (#1765 (comment)): 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
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).
…her's spike 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.
PLG+DOCS: plugins dev docs for temp files + adguard export cleanup
Add pihole_monitor plugin: combined Pi-hole device import + query anomaly detection
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.