From d9ef76d9429c5cc4c2f49b1a16610f579cb26b3e Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Thu, 24 Sep 2026 10:26:24 +1000 Subject: [PATCH] BE: plugin input improvements --- .claude/skills/plugin-review/SKILL.md | 2 +- .gemini/skills/plugin-review/SKILL.md | 2 +- .github/skills/plugin-review/SKILL.md | 2 +- server/plugin.py | 10 +++- .../test_plugin_object_field_sanitization.py | 52 +++++++++++++++++++ 5 files changed, 64 insertions(+), 4 deletions(-) diff --git a/.claude/skills/plugin-review/SKILL.md b/.claude/skills/plugin-review/SKILL.md index b7f4a3f10..330658807 100644 --- a/.claude/skills/plugin-review/SKILL.md +++ b/.claude/skills/plugin-review/SKILL.md @@ -39,6 +39,6 @@ A plugin that parses data from an unauthenticated peer (DHCP options, mDNS/Avahi At minimum, every such detection needs `mylog("none", ...)` (`logger.py`'s `debugLevels` — `"none"` is level 0, the always-shown floor, not filtered out at any configured `LOG_LEVEL` — matching how this codebase already logs real errors, e.g. `mylog("none", f"[Plugins] ⚠ ERROR: {e}")`). A silently-dropped or silently-mangled value with no log trace is the finding to raise — an admin investigating "why does this device's name look wrong" or "was my network probed" has nothing to go on otherwise. -**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 explicitly ties it to `process_plugin_events()`'s existing `"new"`/`"watched-changed"`/`"watched-not-changed"` per-object status (`server/plugin.py:769-791`) to get repeat-suppression for free. If a PR adds `write_notification()` for this without that gating (or an equivalent), that's the thing to flag — not the absence of a user-facing alert on its own. +**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. diff --git a/.gemini/skills/plugin-review/SKILL.md b/.gemini/skills/plugin-review/SKILL.md index b7f4a3f10..330658807 100644 --- a/.gemini/skills/plugin-review/SKILL.md +++ b/.gemini/skills/plugin-review/SKILL.md @@ -39,6 +39,6 @@ A plugin that parses data from an unauthenticated peer (DHCP options, mDNS/Avahi At minimum, every such detection needs `mylog("none", ...)` (`logger.py`'s `debugLevels` — `"none"` is level 0, the always-shown floor, not filtered out at any configured `LOG_LEVEL` — matching how this codebase already logs real errors, e.g. `mylog("none", f"[Plugins] ⚠ ERROR: {e}")`). A silently-dropped or silently-mangled value with no log trace is the finding to raise — an admin investigating "why does this device's name look wrong" or "was my network probed" has nothing to go on otherwise. -**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 explicitly ties it to `process_plugin_events()`'s existing `"new"`/`"watched-changed"`/`"watched-not-changed"` per-object status (`server/plugin.py:769-791`) to get repeat-suppression for free. If a PR adds `write_notification()` for this without that gating (or an equivalent), that's the thing to flag — not the absence of a user-facing alert on its own. +**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. diff --git a/.github/skills/plugin-review/SKILL.md b/.github/skills/plugin-review/SKILL.md index 4cdcf1f4e..eb1cb9081 100644 --- a/.github/skills/plugin-review/SKILL.md +++ b/.github/skills/plugin-review/SKILL.md @@ -39,6 +39,6 @@ A plugin that parses data from an unauthenticated peer (DHCP options, mDNS/Avahi At minimum, every such detection needs `mylog("none", ...)` (`logger.py`'s `debugLevels` — `"none"` is level 0, the always-shown floor, not filtered out at any configured `LOG_LEVEL` — matching how this codebase already logs real errors, e.g. `mylog("none", f"[Plugins] ⚠ ERROR: {e}")`). A silently-dropped or silently-mangled value with no log trace is the finding to raise — an admin investigating "why does this device's name look wrong" or "was my network probed" has nothing to go on otherwise. -**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 explicitly ties it to `process_plugin_events()`'s existing `"new"`/`"watched-changed"`/`"watched-not-changed"` per-object status (`server/plugin.py:769-791`) to get repeat-suppression for free. If a PR adds `write_notification()` for this without that gating (or an equivalent), that's the thing to flag — not the absence of a user-facing alert on its own. +**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. diff --git a/server/plugin.py b/server/plugin.py index 0cad0a3b8..104eea9bd 100755 --- a/server/plugin.py +++ b/server/plugin.py @@ -1208,6 +1208,12 @@ class plugin_object_class: (8, "watchedValue3"), (9, "watchedValue4"), ] + indexAttrMapping = { + 6: "watched1", + 7: "watched2", + 8: "watched3", + 9: "watched4", + } if setObj is not None: self.watchedClmns = setObj["value"] @@ -1217,9 +1223,11 @@ class plugin_object_class: if clmName == mapping[1]: self.watchedIndxs.append(mapping[0]) + # Use the sanitized watched1-4 attributes, not the raw objDbRow values, + # so two events exposing the same sanitized value hash identically. tmp = "" for indx in self.watchedIndxs: - tmp += str(objDbRow[indx]) + tmp += str(getattr(self, indexAttrMapping[indx])) self.watchedHash = str(hash(tmp)) diff --git a/test/server/test_plugin_object_field_sanitization.py b/test/server/test_plugin_object_field_sanitization.py index 408a15fce..3f0f8f760 100644 --- a/test/server/test_plugin_object_field_sanitization.py +++ b/test/server/test_plugin_object_field_sanitization.py @@ -54,6 +54,58 @@ def _publisher_plugin(allow_raw_text_on_watched2=False): } +def _watched_publisher_plugin(): + """Same shape as _publisher_plugin, but declares watchedValue1 as a WATCH + column so watchedIndxs/watchedHash actually get populated.""" + plugin = _publisher_plugin() + plugin["settings"] = [{"function": "WATCH", "value": ["watchedValue1"]}] + return plugin + + +def _non_mac_identity_plugin(): + """A non-MAC-primaryId plugin (e.g. sync.py's GUID-keyed objects) that + declares objectPrimaryId/objectSecondaryId, so both go through the + generic sanitize loop instead of normalize_mac().""" + return { + "unique_prefix": PREFIX, + "settings": [], + "database_column_definitions": [ + {"column": "objectPrimaryId", "type": "text"}, + {"column": "objectSecondaryId", "type": "text"}, + ], + } + + +class TestIdsHashUsesSanitizedValues: + def test_ids_hash_reflects_sanitized_primary_id_not_raw(self): + """Same risk class as watchedHash: idsHash drives the merge/dedup + loop in process_plugin_events(), so it must be computed from the + sanitized primaryId/secondaryId, not a bypassed raw value.""" + plugin = _non_mac_identity_plugin() + clean = plugin_object_class(plugin, make_plugin_event_row(PREFIX, "hello")) + dirty = plugin_object_class(plugin, make_plugin_event_row(PREFIX, "")) + assert clean.primaryId == dirty.primaryId == "hello" + assert clean.idsHash == dirty.idsHash + + +class TestWatchedHashUsesSanitizedValues: + def test_watched_hash_reflects_sanitized_value_not_raw(self): + """Two raw watched1 values that sanitize to the same text must + produce the same watchedHash - it has to be computed from self.watched1 + (post-sanitization), not objDbRow's raw value.""" + plugin = _watched_publisher_plugin() + clean = plugin_object_class(plugin, make_plugin_event_row(PREFIX, "id1", watched1="hello")) + dirty = plugin_object_class(plugin, make_plugin_event_row(PREFIX, "id1", watched1="")) + assert clean.watched1 == dirty.watched1 == "hello" + assert clean.watchedHash == dirty.watchedHash + + def test_watched_hash_still_differs_for_genuinely_different_values(self): + plugin = _watched_publisher_plugin() + a = plugin_object_class(plugin, make_plugin_event_row(PREFIX, "id1", watched1="hello")) + b = plugin_object_class(plugin, make_plugin_event_row(PREFIX, "id1", watched1="world")) + assert a.watchedHash != b.watchedHash + + class TestDefaultSanitization: def test_watched_field_without_allow_raw_text_is_sanitized(self): row = make_plugin_event_row(PREFIX, "id1", watched2=PAYLOAD)