From f30d27db13406d1d2d1836206690d2de28e6aaf8 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Fri, 25 Sep 2026 08:32:03 +1000 Subject: [PATCH] BE: plugin input improvements --- .claude/skills/plugin-development/SKILL.md | 2 + .claude/skills/plugin-review/SKILL.md | 14 +++- .../skills/plugin-development/plugin-skill.md | 2 + .gemini/skills/plugin-review/SKILL.md | 14 +++- .github/skills/plugin-review/SKILL.md | 14 +++- .../skills/plugin-run-development/SKILL.md | 2 + .github/workflows/code-checks.yml | 35 ++++++++++ docs/PLUGINS_DEV.md | 1 + install/proxmox/requirements.txt | 7 ++ install/ubuntu24/requirements.txt | 7 ++ scripts/check_dependency_mirroring.py | 66 +++++++++++++++++++ server/plugin.py | 8 ++- test/server/test_plugin_identity_collision.py | 20 ++++++ 13 files changed, 187 insertions(+), 5 deletions(-) create mode 100644 scripts/check_dependency_mirroring.py diff --git a/.claude/skills/plugin-development/SKILL.md b/.claude/skills/plugin-development/SKILL.md index 4d404d4ce..eb7ee159e 100644 --- a/.claude/skills/plugin-development/SKILL.md +++ b/.claude/skills/plugin-development/SKILL.md @@ -75,6 +75,8 @@ Every mapped field (`objectPrimaryId`/`objectSecondaryId`/`watchedValue1-4`/`ext Check the plugin against the [Conventions Checklist](../../../docs/PLUGINS_DEV.md#conventions-checklist) — `RUN` default, schedule precedent, `RUN_TIMEOUT` semantics, reusing core settings instead of duplicating them, description length (renders in the Settings UI — keep it short), the multi-instance settings pattern (nested array + popup-form, see `rest_import`, not a hardcoded "primary"/"secondary" pair), and `allow_raw_text` restricted to display-only column types. Most plugin PR review comments trace back to one of these, and `test/plugins/test_plugin_conventions.py` mechanically enforces the RUN-default, description-length, hardcoded-default-drift, RUN_TIMEOUT-reuse-in-loop, array/object dataType-default_value-mismatch, and allow_raw_text-type-restriction items — run it after touching a plugin. +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 `server/plugins/__template/` and customize. Read `docs/PLUGINS_DEV.md` for the full development guide. diff --git a/.claude/skills/plugin-review/SKILL.md b/.claude/skills/plugin-review/SKILL.md index 330658807..555135eaa 100644 --- a/.claude/skills/plugin-review/SKILL.md +++ b/.claude/skills/plugin-review/SKILL.md @@ -1,6 +1,6 @@ --- name: 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. diff --git a/.gemini/skills/plugin-development/plugin-skill.md b/.gemini/skills/plugin-development/plugin-skill.md index 2f1666225..76a281b11 100644 --- a/.gemini/skills/plugin-development/plugin-skill.md +++ b/.gemini/skills/plugin-development/plugin-skill.md @@ -87,6 +87,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. Read `docs/PLUGINS_DEV.md` for the full development guide. diff --git a/.gemini/skills/plugin-review/SKILL.md b/.gemini/skills/plugin-review/SKILL.md index 330658807..555135eaa 100644 --- a/.gemini/skills/plugin-review/SKILL.md +++ b/.gemini/skills/plugin-review/SKILL.md @@ -1,6 +1,6 @@ --- name: 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. diff --git a/.github/skills/plugin-review/SKILL.md b/.github/skills/plugin-review/SKILL.md index eb1cb9081..5e1b06bd1 100644 --- a/.github/skills/plugin-review/SKILL.md +++ b/.github/skills/plugin-review/SKILL.md @@ -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. diff --git a/.github/skills/plugin-run-development/SKILL.md b/.github/skills/plugin-run-development/SKILL.md index 2d4ef519d..a968e6940 100644 --- a/.github/skills/plugin-run-development/SKILL.md +++ b/.github/skills/plugin-run-development/SKILL.md @@ -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. diff --git a/.github/workflows/code-checks.yml b/.github/workflows/code-checks.yml index b80fb8590..20e291ab1 100644 --- a/.github/workflows/code-checks.yml +++ b/.github/workflows/code-checks.yml @@ -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 }}" diff --git a/docs/PLUGINS_DEV.md b/docs/PLUGINS_DEV.md index 290512cc7..0c25e5f7e 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 - **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) — not mechanically enforced by `test_plugin_conventions.py`, so review it by eye. Omitting it leaves `scanSourcePlugin` `NULL` on every row this plugin inserts, which silently breaks two things in `server/scan/device_handling.py`: `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. - **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/install/proxmox/requirements.txt b/install/proxmox/requirements.txt index 8f6b050bf..8cb2e3481 100755 --- a/install/proxmox/requirements.txt +++ b/install/proxmox/requirements.txt @@ -1,3 +1,4 @@ +cryptography<40 openwrt-luci-rpc asusrouter aiohttp @@ -23,6 +24,12 @@ dnspython librouteros yattag zeroconf +simplejson +future +six +urllib3 +httplib2 psutil freebox-api +pydantic>=2.0,<3.0 fritzconnection>=1.15.1 diff --git a/install/ubuntu24/requirements.txt b/install/ubuntu24/requirements.txt index 8f6b050bf..8cb2e3481 100755 --- a/install/ubuntu24/requirements.txt +++ b/install/ubuntu24/requirements.txt @@ -1,3 +1,4 @@ +cryptography<40 openwrt-luci-rpc asusrouter aiohttp @@ -23,6 +24,12 @@ dnspython librouteros yattag zeroconf +simplejson +future +six +urllib3 +httplib2 psutil freebox-api +pydantic>=2.0,<3.0 fritzconnection>=1.15.1 diff --git a/scripts/check_dependency_mirroring.py b/scripts/check_dependency_mirroring.py new file mode 100644 index 000000000..d00cfba0e --- /dev/null +++ b/scripts/check_dependency_mirroring.py @@ -0,0 +1,66 @@ +#!/usr/bin/env python3 +""" +Flag PRs that add a system package (root Dockerfile) or a Python dependency +(root requirements.txt) without touching the sibling build targets that also +need it. See docs/PLUGINS_DEV.md#conventions-checklist and the +plugin-review skill's "a new dependency has to reach every build target" +check - a package added only to the Alpine Dockerfile leaves Dockerfile.debian +(a separately-maintained apt-based target, docs/BUILDS.md) without it, and a +Python package added only to the root requirements.txt may still be needed by +install/ubuntu24 or install/proxmox's own non-Docker install methods. + +This can't know whether a given package is actually needed on the other +target (a real difference is legitimate and common), only that the PR didn't +touch the sibling file at all - a signal worth a human glance, not a +mechanical verdict. Exit non-zero (the CI step calling this is non-blocking). + + python3 scripts/check_dependency_mirroring.py origin/main +""" + +import subprocess +import sys + +GROUPS = [ + ["Dockerfile", "Dockerfile.debian"], + ["requirements.txt", "install/ubuntu24/requirements.txt", "install/proxmox/requirements.txt"], +] + + +def changed_files(base_ref): + result = subprocess.run( + ["git", "diff", "--name-only", f"{base_ref}...HEAD"], + capture_output=True, text=True, check=True, + ) + return set(result.stdout.splitlines()) + + +def main(): + if len(sys.argv) != 2: + print("usage: check_dependency_mirroring.py ", file=sys.stderr) + return 2 + + changed = changed_files(sys.argv[1]) + problems = [] + for group in GROUPS: + touched = [path for path in group if path in changed] + untouched = [path for path in group if path not in changed] + if touched and untouched: + problems.append( + f"- touched {', '.join(touched)} but not {', '.join(untouched)}." + ) + + if problems: + print("Possible dependency-mirroring gap (a new package may need the other file(s) too):") + print("\n".join(problems)) + print("\nIf the change is genuinely target-specific (e.g. a Debian-only fix, or a " + "dependency only the Docker build needs), ignore this. Otherwise check whether " + "the other file(s) need the same package - see " + "docs/PLUGINS_DEV.md#conventions-checklist.") + return 1 + + print("No dependency-mirroring gap detected.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/server/plugin.py b/server/plugin.py index 48a67912f..62927b505 100755 --- a/server/plugin.py +++ b/server/plugin.py @@ -1216,8 +1216,12 @@ class plugin_object_class: objDbRow, ) - self.idsHash = str(hash(str(self.primaryId) + str(self.secondaryId))) - # self.idsHash = str(self.primaryId) + str(self.secondaryId) + # Hashing the pair, not their concatenation - str(primaryId) + str(secondaryId) + # would make ("ab", "c") and ("a", "bc") produce the identical string "abc" + # and therefore the identical hash, wrongly treating two distinct identities + # as one (and, since seen_by_hash above compares idsHash too, wrongly + # rejecting a legitimate batch as a collision). + self.idsHash = str(hash((str(self.primaryId), str(self.secondaryId)))) self.watchedClmns = [] self.watchedIndxs = [] diff --git a/test/server/test_plugin_identity_collision.py b/test/server/test_plugin_identity_collision.py index ea4b9c245..01e12a925 100644 --- a/test/server/test_plugin_identity_collision.py +++ b/test/server/test_plugin_identity_collision.py @@ -127,6 +127,26 @@ class TestNoCollisionBehavesAsBefore: ids = {r[2] for r in rows} assert ids == {"device_A", "device_B"} + def test_concatenation_boundary_shift_is_not_a_false_collision(self, plugin_db, monkeypatch): + """idsHash hashes the (primaryId, secondaryId) pair, not their string + concatenation - ("ab", "c") and ("a", "bc") both concatenate to "abc", + which would wrongly hash identically and trip the collision guard, + rejecting a legitimate batch that has no real duplicate in it.""" + db, conn = plugin_db + monkeypatch.setattr("plugin.get_setting_value", _no_report_on) + + plugin = make_plugin_dict(PREFIX) + events = [ + make_plugin_event_row(PREFIX, "ab", secondary_id="c"), + make_plugin_event_row(PREFIX, "a", secondary_id="bc"), + ] + + process_plugin_events(db, plugin, events) + + rows = plugin_objects_rows(conn, PREFIX) + pairs = {(r[2], r[3]) for r in rows} # (objectPrimaryId, objectSecondaryId) + assert pairs == {("ab", "c"), ("a", "bc")} + def test_event_matching_a_preexisting_object_is_not_a_collision(self, plugin_db, monkeypatch): """The collision check only compares events against each other, not against pre-existing Plugins_Objects rows - matching an existing