mirror of
https://github.com/jokob-sk/NetAlertX.git
synced 2026-10-02 18:55:04 -04:00
BE: plugin input improvements
This commit is contained in:
1 parent
33003a4c4d
commit
f30d27db13
13 files changed
+187
-5
No files matched your search
@@ -1,6 +1,6 @@
|
||||
---
|
||||
name: netalertx-plugin-review
|
||||
description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers two checks not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method, and suspicious/attacker-influenced plugin data not being logged - plus worked real-PR examples.
|
||||
description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers three checks not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method, suspicious/attacker-influenced plugin data not being logged, and a new dependency not reaching every build target - plus worked real-PR examples.
|
||||
---
|
||||
|
||||
# Plugin Review
|
||||
@@ -42,3 +42,15 @@ At minimum, every such detection needs `mylog("none", ...)` (`logger.py`'s `debu
|
||||
**A user-facing alert (`write_notification()`, `server/messaging/in_app.py`) is a separate, materially bigger decision: don't require it as a blocking condition the way the log line is.** It's persistent and unprompted, and (per existing precedent: `api_server_start.py`'s unauthorized-access-attempt alert fires unconditionally, with no rate-limiting anywhere in this codebase) a repeat offender re-sending the same payload every scan cycle can spam it indefinitely unless the PR gates it correctly on `process_plugin_events()`'s existing per-object status (`server/plugin.py:769-791`): fire only on `"new"` or `"watched-changed"`, never on `"watched-not-changed"` (that status is set every cycle a value stays the same, so alerting on it defeats the suppression entirely). For a missing-object alert, fire only on the transition into `"missing-in-last-scan"` (`server/plugin.py`'s `if tmpObj.status != "missing-in-last-scan":` guard around line 807), not on every cycle the object remains in that status. If a PR adds `write_notification()` for this without matching that gating, that's the thing to flag, not the absence of a user-facing alert on its own.
|
||||
|
||||
**Worked example (generalized):** a plugin parses an unauthenticated broadcast-protocol response (e.g. a DHCP option, an mDNS/NetBIOS record) and copies a field from it verbatim into a stored value with no validation. The fix centralizes both the sanitization *and* the `mylog("none", ...)` call in one shared, plugin-agnostic enforcement point (`plugin_object_class.__init__`, `server/plugin.py`) rather than leaving individual plugin authors to remember either — the same reasoning as the raw-SQL check above: a check that depends on every plugin author independently thinking to add it will eventually ship without it.
|
||||
|
||||
## The third check this skill adds: a new dependency has to reach every build target the plugin should run on
|
||||
|
||||
If a PR adds a system package (`apk add` in the root `Dockerfile`) or a Python dependency (`requirements.txt`), check whether it actually reached every place that needs it, not just the one file the diff touched:
|
||||
|
||||
- **`.devcontainer/Dockerfile`** is a committed, git-tracked file generated by concatenating the root `Dockerfile` with `.devcontainer/resources/devcontainer-Dockerfile` (`.devcontainer/scripts/generate-configs.sh`). It is not read fresh from the root `Dockerfile` at build time, so a PR that edits the root `Dockerfile` without re-running that script leaves this file stale, silently missing the new package for anyone testing inside the devcontainer. CI enforces this: the `check-devcontainer-dockerfile` job (`.github/workflows/code-checks.yml`) regenerates the file and fails the build if it doesn't match the committed one.
|
||||
- **`Dockerfile.debian`** (`docs/BUILDS.md`) is a second, separately-maintained build target with its own `apt-get install` list and `setcap` calls. A system package (and its capability grant, if the plugin needs one, like `arp-scan`/`nmap`/`iw`) added only to the Alpine `Dockerfile` leaves this target broken.
|
||||
- **`install/ubuntu24/requirements.txt`** and **`install/proxmox/requirements.txt`** are separate Python dependency lists for their own non-Docker install methods, not mechanically synced with the root `requirements.txt` (they already drift from it today) - check whether the new dependency is actually needed by those install paths too.
|
||||
|
||||
For the latter two, `scripts/check_dependency_mirroring.py` (wired into the non-blocking `check-dependency-mirroring` CI job) flags a PR that touches `Dockerfile` without `Dockerfile.debian`, or `requirements.txt` without both `install/*` copies - it only knows the sibling file wasn't touched at all, not whether the specific package was actually needed there, so treat a flag as a prompt to check, not a verdict.
|
||||
|
||||
A worked example: a WiFi-scanning plugin PR added `iw` plus its `setcap` grant to the root `Dockerfile` only. The devcontainer's checked-in Dockerfile went stale (missing `iw` until someone regenerates it), and `Dockerfile.debian` never got `iw` or a `setcap` line for it at all - the plugin silently can't scan on either target, caught only because the plugin's own error handling logs "not found" rather than crashing.
|
||||
@@ -88,6 +88,8 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext
|
||||
|
||||
Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) in `docs/PLUGINS_DEV.md` — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings, description length, the multi-instance settings pattern, and `allow_raw_text` restricted to display-only column types. Most plugin PR review comments trace back to one of these.
|
||||
|
||||
If the plugin needs a new system package or Python dependency, mirroring it into the root `Dockerfile`/`requirements.txt` alone is not enough: see the Conventions Checklist's build-target-mirroring bullet for `.devcontainer/Dockerfile` (regenerate via `.devcontainer/scripts/generate-configs.sh`, don't hand-edit it), `Dockerfile.debian`, and `install/ubuntu24`/`install/proxmox`'s own `requirements.txt` files.
|
||||
|
||||
## Starting Point
|
||||
|
||||
Copy from `server/plugins/__template` and customize.
|
||||
@@ -127,3 +127,38 @@ jobs:
|
||||
- name: 🔍 Check for skill-pair drift
|
||||
continue-on-error: true
|
||||
run: python3 scripts/check_skill_pairs.py "origin/${{ github.base_ref }}"
|
||||
|
||||
check-devcontainer-dockerfile:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- name: Checkout code
|
||||
uses: actions/checkout@v4
|
||||
|
||||
- name: 🔍 Check .devcontainer/Dockerfile is up to date
|
||||
run: |
|
||||
echo "Regenerating .devcontainer/Dockerfile from the root Dockerfile + .devcontainer/resources/devcontainer-Dockerfile..."
|
||||
cp .devcontainer/Dockerfile /tmp/devcontainer-dockerfile-committed
|
||||
bash .devcontainer/scripts/generate-configs.sh
|
||||
|
||||
if ! diff -q /tmp/devcontainer-dockerfile-committed .devcontainer/Dockerfile > /dev/null; then
|
||||
echo "❌ .devcontainer/Dockerfile is stale."
|
||||
echo "It's generated from the root Dockerfile and .devcontainer/resources/devcontainer-Dockerfile,"
|
||||
echo "not hand-edited. Run 'bash .devcontainer/scripts/generate-configs.sh' locally and commit the result."
|
||||
echo
|
||||
diff /tmp/devcontainer-dockerfile-committed .devcontainer/Dockerfile || true
|
||||
exit 1
|
||||
fi
|
||||
echo "✅ .devcontainer/Dockerfile is up to date."
|
||||
|
||||
check-dependency-mirroring:
|
||||
if: github.event_name == 'pull_request'
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- name: Checkout code
|
||||
uses: actions/checkout@v4
|
||||
with:
|
||||
fetch-depth: 0
|
||||
|
||||
- name: 🔍 Check for dependency-mirroring gaps
|
||||
continue-on-error: true
|
||||
run: python3 scripts/check_dependency_mirroring.py "origin/${{ github.base_ref }}"
|
||||
Reference in new issue
Block a user