BE: plugin input improvements

.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.

.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.

.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.

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))
 

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, "<hello>"))
+        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="<hello>"))
+        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)