Merge branch 'next_release' of github.com:netalertx/NetAlertX into next_release

.github/skills/code-standards/SKILL.md

@@ -98,6 +98,15 @@ from db_test_helpers import make_db, DummyDB, insert_device, minutes_ago
 
 If a helper you need doesn't exist yet, add it to `db_test_helpers.py` — not locally in the test file.
 
+## Stubbing Modules in Standalone-Capable Tests
+
+If a test stubs NetAlertX modules into `sys.modules` so a script can be imported
+outside the container (see `test/plugins/test_ntfy_custom_headers.py`), pop each
+stubbed name back out of `sys.modules` right after the one-time import that needed
+it. Otherwise the fake module leaks into every other test file collected in the
+same pytest session and shadows the real module (see `testing-workflow` skill for
+the full pattern and reproduction steps).
+
 ## MAC Literals in Tests — ALWAYS Lowercase
 
 **MANDATORY:** Every MAC address literal used in test fixtures, parametrize decorators, assertions, or comments must be lowercase hex:

.github/skills/testing-workflow/SKILL.md

@@ -59,3 +59,49 @@ docker buildx build -t netalertx-test .
 ```
 
 This takes ~30 seconds unless venv stage changes (~90s).
+
+## Pitfall: `sys.modules` Stubbing Leaks Across Test Files
+
+Some plugin tests (e.g. `test/plugins/test_ntfy_custom_headers.py`) stub NetAlertX
+modules (`conf`, `helper`, `models.notification_instance`, etc.) via
+`sys.modules[name] = fake_module` so the plugin script can be imported standalone,
+outside the container. Because `sys.modules` is a single process-wide cache shared
+by the whole pytest session, a fake module inserted by one test file silently
+shadows the real module for every other test file collected afterwards — pytest
+imports all test files during collection, before any test runs, so this can happen
+regardless of alphabetical/directory order.
+
+Symptom: `AttributeError: <module 'models.notification_instance'> does not have
+the attribute 'get_setting_value'` (or similar) in an unrelated test file, where
+the module repr has no `from '<path>'` suffix — a giveaway that a stub, not the
+real module, was resolved.
+
+Fix pattern: track which module names your stub actually inserted, and pop them
+back out of `sys.modules` immediately after the one-time import that needed them
+(the already-imported script keeps its bound names regardless):
+
+```python
+_stubbed_module_names = []
+
+def _stub(name, **attrs):
+    if name not in sys.modules:
+        mod = types.ModuleType(name)
+        for k, v in attrs.items():
+            setattr(mod, k, v)
+        sys.modules[name] = mod
+        _stubbed_module_names.append(name)
+
+# ... _stub(...) calls, then the one-time import ...
+import ntfy
+
+for _name in _stubbed_module_names:
+    sys.modules.pop(_name, None)
+```
+
+Reproduce cross-file pollution locally by running the suspect file together with
+the affected one in a single pytest invocation (order matters less than you'd
+think — collection happens for all files first):
+
+```bash
+pytest test/plugins/test_ntfy_custom_headers.py test/backend/test_notification_templates.py -v
+```

front/js/network-tabs.js

@@ -13,7 +13,7 @@ function renderNetworkTabs(nodes) {
                       (node.devAlertDown == 1 ? "text-red" : "text-gray50"));
 
     const portLabel = node.node_ports_count ? ` (${node.node_ports_count})` : '';
-    const icon = atob(node.devIcon);
+    const icon = safeAtob(node.devIcon);
     const id = node.devMac.replace(/:/g, '_');
 
     html += `

front/js/ui_components.js

@@ -971,6 +971,9 @@ function renderDeviceLink(data, container, useName = false) {
   // Build and return badge parts
   const badge = badgeFromDevice(device);
 
+  // Decode once (with a safe fallback) and reuse for both the chip and hover preview
+  const decodedIcon = safeAtob(device.devIcon);
+
   // badge class and hover-info class to container
   $(container)
     .addClass(`${badge.cssClass} hover-node-info`)
@@ -989,14 +992,14 @@ function renderDeviceLink(data, container, useName = false) {
       'data-alertdown': device.devAlertDown,
       'data-sleeping': device.devIsSleeping || 0,
       'data-archived': device.devIsArchived || 0,
-      'data-isnew':    device.devIsNew       || 0,
-      'data-icon': device.devIcon
+      'data-isnew':    device.devIsNew      || 0,
+      'data-icon': decodedIcon
     });
 
   return `
     <a href="${badge.url}" target="_blank">
       <span class="custom-chip">
-        <span class="iconPreview">${atob(device.devIcon)}</span>
+        <span class="iconPreview">${decodedIcon}</span>
         ${useName ? encodeSpecialChars(device.devName) : data.text}
         <span>
           (${badge.iconHtml})
@@ -1006,6 +1009,17 @@ function renderDeviceLink(data, container, useName = false) {
   `;
 }
 
+// ------------------------------------------
+// Base64-decode a devIcon value, tolerating missing/empty/malformed input
+function safeAtob(value) {
+  if (!value) return '';
+  try {
+    return atob(value);
+  } catch (e) {
+    return '';
+  }
+}
+
 // ------------------------------------------
 // Display device info on hover (attach only once)
 function initHoverNodeInfo() {
@@ -1063,7 +1077,7 @@ function initHoverNodeInfo() {
 
       const html = `
         <div>
-          <b> <div class="iconPreview">${atob(icon)}</div> </b><b class="devName"> ${encodeSpecialChars(name)}</b><br>
+          <b> <div class="iconPreview">${icon || ''}</div> </b><b class="devName"> ${encodeSpecialChars(name)}</b><br>
         </div>
         <hr/>
         <div class="line">

server/database.py

@@ -16,6 +16,8 @@ from db.db_upgrade import (
     ensure_Settings,
     ensure_Indexes,
     ensure_mac_lowercase_triggers,
+    ensure_dangling_parentmac_cleanup_trigger,
+    cleanup_existing_dangling_parentmac,
     migrate_to_camelcase,
     migrate_timestamps_to_utc,
 )
@@ -225,6 +227,9 @@ class DB:
             # Normalization triggers
             ensure_mac_lowercase_triggers(self.sql)
 
+            # Prevent/repair dangling devParentMAC references left by deleted devices
+            cleanup_existing_dangling_parentmac(self.sql)
+
             # Device history table + audit triggers
             ensure_deviceshistory_table(self.sql)
             ensure_deviceshistory_triggers(self.sql)
@@ -240,9 +245,10 @@ class DB:
         AppEvent_obj(self)
 
         # AppEvent_obj.drop_all_triggers() wipes every trigger in the DB
-        # (including trg_devhist_*) as part of its clean-start routine.
-        # Re-create the device history audit triggers here so they survive.
+        # (including trg_devhist_* and trg_clear_dangling_parentmac_on_delete)
+        # as part of its clean-start routine. Re-create them here so they survive.
         ensure_deviceshistory_triggers(self.sql)
+        ensure_dangling_parentmac_cleanup_trigger(self.sql)
         self.commitDB()
 
     def get_table_as_json(self, sqlQuery, parameters=None):

server/db/db_upgrade.py

@@ -146,6 +146,74 @@ def ensure_mac_lowercase_triggers(sql):
         return False
 
 
+# Sentinel devParentMAC values that are never actual device references
+PARENT_MAC_SENTINELS = ("", "internet", "null")
+
+
+def ensure_dangling_parentmac_cleanup_trigger(sql):
+    """
+    Ensures a trigger exists that clears devParentMAC/devParentMACSource on any
+    device that referenced a device MAC which was just deleted, preventing
+    dangling Parent Node references.
+
+    Note: this intentionally does NOT touch the NEWDEV_devParentMAC setting.
+    Settings are sourced from app.conf and get re-imported verbatim on every
+    restart (see importConfigs()), so a DB-only fix here would be silently
+    reverted. Stale NEWDEV_devParentMAC values are instead guarded against at
+    the point of use in create_new_devices() (server/scan/device_handling.py).
+    """
+    try:
+        sql.execute(
+            "SELECT name FROM sqlite_master WHERE type='trigger' AND name='trg_clear_dangling_parentmac_on_delete'"
+        )
+        if not sql.fetchone():
+            mylog("verbose", ["[db_upgrade] Creating trigger 'trg_clear_dangling_parentmac_on_delete'"])
+            sql.execute("""
+                CREATE TRIGGER trg_clear_dangling_parentmac_on_delete
+                AFTER DELETE ON Devices
+                FOR EACH ROW
+                WHEN OLD.devMac IS NOT NULL AND OLD.devMac != ''
+                BEGIN
+                    UPDATE Devices
+                    SET devParentMAC = '', devParentMACSource = ''
+                    WHERE LOWER(devParentMAC) = LOWER(OLD.devMac);
+                END;
+            """)
+
+        return True
+
+    except Exception as e:
+        mylog("none", [f"[db_upgrade] ERROR while ensuring dangling parentMAC trigger: {e}"])
+        return False
+
+
+def cleanup_existing_dangling_parentmac(sql) -> bool:
+    """
+    One-time/idempotent cleanup for installations that already have devParentMAC
+    values pointing to a MAC no longer present in Devices. The delete trigger
+    only prevents new dangling references going forward, so this repairs data
+    left over from before the trigger existed.
+    """
+    try:
+        sentinel_list = ", ".join(f"'{v}'" for v in PARENT_MAC_SENTINELS)
+
+        sql.execute(f"""
+            UPDATE Devices
+            SET devParentMAC = '', devParentMACSource = ''
+            WHERE devParentMAC IS NOT NULL
+              AND LOWER(devParentMAC) NOT IN ({sentinel_list})
+              AND LOWER(devParentMAC) NOT IN (SELECT LOWER(devMac) FROM Devices)
+        """)
+        if sql.rowcount > 0:
+            mylog("verbose", [f"[db_upgrade] Cleared {sql.rowcount} dangling devParentMAC reference(s)"])
+
+        return True
+
+    except Exception as e:
+        mylog("none", [f"[db_upgrade] ERROR while cleaning up dangling parentMAC references: {e}"])
+        return False
+
+
 def ensure_views(sql) -> bool:
     """
     Ensures required views exist.

server/scan/device_handling.py

@@ -10,6 +10,7 @@ from models.device_instance import DeviceInstance
 from scan.name_resolution import NameResolver
 from scan.device_heuristics import guess_icon, guess_type
 from db.db_helper import sanitize_SQL_input, list_to_where, safe_int
+from db.db_upgrade import PARENT_MAC_SENTINELS
 from db.authoritative_handler import (
     get_overwrite_sql_clause,
     can_overwrite_field,
@@ -730,6 +731,22 @@ def create_new_devices(db):
     mylog("debug", f"[New Devices] Collecting New Devices Query: {query}")
     current_scan_data = sql.execute(query).fetchall()
 
+    # Resolve the default Parent Node setting once and guard against it pointing
+    # to a MAC that no longer exists (e.g. that device was since deleted) -
+    # falling back to unset rather than seeding new devices with a dangling reference.
+    default_parent_mac_setting = get_setting_value("NEWDEV_devParentMAC")
+    if default_parent_mac_setting and default_parent_mac_setting.lower() not in PARENT_MAC_SENTINELS:
+        existing_device_macs = {
+            str(row[0]).lower() for row in sql.execute("SELECT devMac FROM Devices").fetchall() if row[0]
+        }
+        if default_parent_mac_setting.lower() not in existing_device_macs:
+            mylog(
+                "verbose",
+                f"[New Devices] NEWDEV_devParentMAC '{default_parent_mac_setting}' no longer "
+                "exists in Devices - treating as unset",
+            )
+            default_parent_mac_setting = ""
+
     for row in current_scan_data:
         (
             scanMac,
@@ -771,7 +788,7 @@ def create_new_devices(db):
             scanParentMAC
             if scanParentMAC and scanMac.lower() != "internet"
             else (
-                get_setting_value("NEWDEV_devParentMAC")
+                default_parent_mac_setting
                 if scanMac.lower() != "internet"
                 else "null"
             )

test/db/test_dangling_parentmac_cleanup.py

@@ -0,0 +1,172 @@
+"""
+Unit tests for dangling devParentMAC cleanup.
+
+Tests verify that:
+- Deleting a device clears devParentMAC/devParentMACSource on devices that
+  referenced it as their Parent Node.
+- Sentinel values ('', 'internet', 'null') are never touched.
+- Valid parent references are left untouched.
+- The one-time migration repairs pre-existing dangling data and is idempotent.
+
+Note: the NEWDEV_devParentMAC *setting* is intentionally NOT handled here.
+Settings are sourced from app.conf and get re-imported verbatim on every
+restart, so a DB-only fix would be silently reverted. That case is instead
+guarded against at the point of use in create_new_devices() — see
+test/scan/test_field_lock_scan_integration.py.
+"""
+
+import sys
+import os
+import pytest
+import sqlite3
+import tempfile
+
+INSTALL_PATH = os.getenv('NETALERTX_APP', '/app')
+sys.path.extend([f"{INSTALL_PATH}/server/plugins", f"{INSTALL_PATH}/server"])
+
+from db.db_upgrade import (  # noqa: E402
+    ensure_dangling_parentmac_cleanup_trigger,
+    cleanup_existing_dangling_parentmac,
+)
+
+
+@pytest.fixture
+def temp_db():
+    """Create a temporary database for testing"""
+    fd, db_path = tempfile.mkstemp(suffix='.db')
+    os.close(fd)
+
+    conn = sqlite3.connect(db_path)
+    cursor = conn.cursor()
+
+    cursor.execute("""
+        CREATE TABLE Devices (
+            devMac TEXT PRIMARY KEY COLLATE NOCASE,
+            devParentMAC TEXT,
+            devParentMACSource TEXT
+        )
+    """)
+
+    conn.commit()
+
+    yield cursor, conn
+
+    conn.close()
+    os.unlink(db_path)
+
+
+class TestDanglingParentMacTrigger:
+    """Test suite for the AFTER DELETE cleanup trigger"""
+
+    def test_trigger_clears_dependent_devices_on_delete(self, temp_db):
+        cursor, conn = temp_db
+        assert ensure_dangling_parentmac_cleanup_trigger(cursor) is True
+
+        cursor.execute(
+            "INSERT INTO Devices (devMac, devParentMAC, devParentMACSource) VALUES (?, ?, ?)",
+            ("aa:bb:cc:dd:ee:01", "", ""),
+        )
+        cursor.execute(
+            "INSERT INTO Devices (devMac, devParentMAC, devParentMACSource) VALUES (?, ?, ?)",
+            ("aa:bb:cc:dd:ee:02", "aa:bb:cc:dd:ee:01", "NEWDEV"),
+        )
+        conn.commit()
+
+        cursor.execute("DELETE FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:01",))
+        conn.commit()
+
+        cursor.execute(
+            "SELECT devParentMAC, devParentMACSource FROM Devices WHERE devMac = ?",
+            ("aa:bb:cc:dd:ee:02",),
+        )
+        row = cursor.fetchone()
+        assert row == ("", "")
+
+    def test_trigger_ignores_unrelated_deletes(self, temp_db):
+        cursor, conn = temp_db
+        ensure_dangling_parentmac_cleanup_trigger(cursor)
+
+        cursor.execute(
+            "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)",
+            ("aa:bb:cc:dd:ee:01", "internet"),
+        )
+        cursor.execute(
+            "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)",
+            ("aa:bb:cc:dd:ee:02", ""),
+        )
+        conn.commit()
+
+        cursor.execute("DELETE FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:02",))
+        conn.commit()
+
+        cursor.execute(
+            "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:01",)
+        )
+        assert cursor.fetchone() == ("internet",)
+
+
+class TestCleanupExistingDanglingParentMac:
+    """Test suite for the one-time/idempotent data repair migration"""
+
+    def test_cleanup_clears_dangling_reference(self, temp_db):
+        cursor, conn = temp_db
+
+        cursor.execute(
+            "INSERT INTO Devices (devMac, devParentMAC, devParentMACSource) VALUES (?, ?, ?)",
+            ("aa:bb:cc:dd:ee:02", "aa:bb:cc:dd:ee:99", "NEWDEV"),
+        )
+        conn.commit()
+
+        assert cleanup_existing_dangling_parentmac(cursor) is True
+
+        cursor.execute(
+            "SELECT devParentMAC, devParentMACSource FROM Devices WHERE devMac = ?",
+            ("aa:bb:cc:dd:ee:02",),
+        )
+        assert cursor.fetchone() == ("", "")
+
+    def test_cleanup_preserves_valid_and_sentinel_values(self, temp_db):
+        cursor, conn = temp_db
+
+        cursor.execute(
+            "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)",
+            ("aa:bb:cc:dd:ee:01", ""),
+        )
+        cursor.execute(
+            "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)",
+            ("aa:bb:cc:dd:ee:02", "aa:bb:cc:dd:ee:01"),
+        )
+        cursor.execute(
+            "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)",
+            ("aa:bb:cc:dd:ee:03", "internet"),
+        )
+        conn.commit()
+
+        cleanup_existing_dangling_parentmac(cursor)
+
+        cursor.execute(
+            "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:02",)
+        )
+        assert cursor.fetchone() == ("aa:bb:cc:dd:ee:01",)
+
+        cursor.execute(
+            "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:03",)
+        )
+        assert cursor.fetchone() == ("internet",)
+
+    def test_cleanup_is_idempotent(self, temp_db):
+        cursor, conn = temp_db
+
+        cursor.execute(
+            "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)",
+            ("aa:bb:cc:dd:ee:02", "aa:bb:cc:dd:ee:99"),
+        )
+        conn.commit()
+
+        assert cleanup_existing_dangling_parentmac(cursor) is True
+        assert cleanup_existing_dangling_parentmac(cursor) is True
+
+        cursor.execute(
+            "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:02",)
+        )
+        assert cursor.fetchone() == ("",)

test/plugins/test_ntfy_custom_headers.py

@@ -16,11 +16,16 @@ from unittest.mock import MagicMock, patch
 
 # ---------------------------------------------------------------------------
 # Stub NetAlertX-specific modules so tests can run outside the container.
-# sys.modules.setdefault() is a no-op when the real module is already loaded,
-# so this is safe to run inside the container too.
+# These stubs are only placeholders for the duration of the `import ntfy`
+# below - they are popped from sys.modules again right after, so they don't
+# leak into other test files sharing the same pytest session (which would
+# otherwise shadow the real modules, e.g. models.notification_instance, for
+# every subsequent test).
 # ---------------------------------------------------------------------------
 _tmp_log = tempfile.mkdtemp()
 
+_stubbed_module_names = []
+
 
 def _stub(name: str, **attrs):
     if name not in sys.modules:
@@ -28,6 +33,7 @@ def _stub(name: str, **attrs):
         for k, v in attrs.items():
             setattr(mod, k, v)
         sys.modules[name] = mod
+        _stubbed_module_names.append(name)
 
 
 _stub("pytz", timezone=lambda tz: tz)
@@ -57,6 +63,13 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server",
 import ntfy  # noqa: E402
 from ntfy import build_custom_headers  # noqa: E402
 
+# `ntfy` has already resolved its module-level `from x import y` bindings at
+# this point, so removing these fake entries from sys.modules doesn't affect
+# it - it just stops them from shadowing the real modules for other test
+# files collected later in the same pytest session.
+for _name in _stubbed_module_names:
+    sys.modules.pop(_name, None)
+
 BUILT_IN = {"Title": "NetAlertX Notification", "Authorization": "Bearer secret"}
 
 

test/scan/test_field_lock_scan_integration.py

@@ -231,6 +231,66 @@ def test_create_new_devices_sets_sources(scan_db_for_new_devices):
     assert row["devVlanSource"] == "NEWDEV"
 
 
+def test_create_new_devices_ignores_dangling_newdev_parentmac(scan_db_for_new_devices):
+    """A stale NEWDEV_devParentMAC pointing to a since-deleted device is treated as unset,
+    instead of seeding the new device with another dangling Parent Node reference."""
+    cur = scan_db_for_new_devices.cursor()
+    cur.execute(
+        """
+        INSERT INTO CurrentScan (
+            scanMac, scanName, scanVendor, scanSourcePlugin, scanLastIP,
+            scanSyncHubNode, scanParentMAC, scanParentPort,
+            scanSite, scanSSID, scanType
+        ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
+        """,
+        (
+            "aa:bb:cc:dd:ee:11",
+            "DeviceTwo",
+            "AcmeVendor",
+            "ARPSCAN",
+            "192.168.1.11",
+            "",
+            "",  # no parent reported by the scan itself
+            "",
+            "",
+            "",
+            "",
+        ),
+    )
+    scan_db_for_new_devices.commit()
+
+    settings = {
+        "NEWDEV_devType": "default-type",
+        # points to a MAC that does not (and never did, in this test) exist in Devices
+        "NEWDEV_devParentMAC": "99:99:99:99:99:99",
+        "NEWDEV_devOwner": "owner",
+        "NEWDEV_devGroup": "group",
+        "NEWDEV_devComments": "",
+        "NEWDEV_devLocation": "",
+        "NEWDEV_devCustomProps": "",
+        "NEWDEV_devParentRelType": "uplink",
+        "SYNC_node_name": "SYNCNODE",
+    }
+
+    db = Mock()
+    db.sql_connection = scan_db_for_new_devices
+    db.sql = cur
+    db.commitDB = scan_db_for_new_devices.commit
+
+    with patch.multiple(
+        device_handling,
+        get_setting_value=Mock(side_effect=lambda key: settings.get(key, "")),
+        safe_int=Mock(return_value=0),
+    ):
+        device_handling.create_new_devices(db)
+
+    row = cur.execute(
+        "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:11",)
+    ).fetchone()
+
+    assert row["devParentMAC"] == ""
+
+
 def test_scan_updates_newdev_device_name(scan_db, mock_device_handlers):
     """Scanner discovers name for device with NEWDEV source."""
     cur = scan_db.cursor()