From a573eeb0e780b51f2a28ebbf62a63ebcd4318ff1 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Tue, 22 Sep 2026 08:43:41 +1000 Subject: [PATCH] BE: FREEBOX dual stack IP fix #1804 --- .claude/skills/scan-pipeline/SKILL.md | 2 +- .gemini/skills/scan-pipeline/SKILL.md | 2 +- .github/skills/scan-pipeline/SKILL.md | 2 +- server/scan/device_handling.py | 54 ++++++++------ test/scan/test_ip_format_and_locking.py | 93 +++++++++++++++++++++++++ 5 files changed, 129 insertions(+), 24 deletions(-) diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index 4d84cfa6..9cbd4a7e 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -34,7 +34,7 @@ Covers what happens after a plugin's rows land in `CurrentScan`: presence comput 6. `update_presence_from_CurrentScan(db)` — sets `devPresentLastScan` from `CurrentScan` for this cycle (step 2 reads this as "previous" on the *next* cycle). 7. `update_devPresentLastScan_based_on_nics(db)` — NIC/parent-child presence aggregation; can override step 6 for parent devices. 8. `update_devPresentLastScan_based_on_force_status(db)` — the user's manual `devForceStatus` override; runs last, wins over everything above. -9. `update_vendors_from_mac`, `update_ipv4_ipv6`, `update_icons_and_types` +9. `update_vendors_from_mac`, `update_ipv4_ipv6`, `update_icons_and_types` — `update_ipv4_ipv6()` does **not** go through `LatestDeviceScan`/`FIELD_SPECS`; it reads `CurrentScan` directly with its own `PARTITION BY scanMac, address_family` ranking, so a device reporting both an IPv4 and an IPv6 row in the same cycle gets both `devPrimaryIPv4`/`devPrimaryIPv6` set from that cycle, not just whichever family happened to win `devLastIP`'s single-value reduction. See `.gemini/internal-docs/PRDs/dual-stack-primary-ip-support.md`. 10. `pair_sessions_events(db)` — pairs `Events` rows as described above. 11. `create_sessions_snapshot(db)` — `DELETE FROM Sessions; INSERT INTO Sessions SELECT * FROM Convert_Events_to_Sessions`. `Sessions` reflects step 10's pairing from here. 12. `insertOnlineHistory(db)` — dashboard graph rollup. diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index 415750fb..0a5be06d 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -34,7 +34,7 @@ Covers what happens after a plugin's rows land in `CurrentScan`: presence comput 6. `update_presence_from_CurrentScan(db)` — sets `devPresentLastScan` from `CurrentScan` for this cycle (step 2 reads this as "previous" on the *next* cycle). 7. `update_devPresentLastScan_based_on_nics(db)` — NIC/parent-child presence aggregation; can override step 6 for parent devices. 8. `update_devPresentLastScan_based_on_force_status(db)` — the user's manual `devForceStatus` override; runs last, wins over everything above. -9. `update_vendors_from_mac`, `update_ipv4_ipv6`, `update_icons_and_types` +9. `update_vendors_from_mac`, `update_ipv4_ipv6`, `update_icons_and_types` — `update_ipv4_ipv6()` does **not** go through `LatestDeviceScan`/`FIELD_SPECS`; it reads `CurrentScan` directly with its own `PARTITION BY scanMac, address_family` ranking, so a device reporting both an IPv4 and an IPv6 row in the same cycle gets both `devPrimaryIPv4`/`devPrimaryIPv6` set from that cycle, not just whichever family happened to win `devLastIP`'s single-value reduction. See `.gemini/internal-docs/PRDs/dual-stack-primary-ip-support.md`. 10. `pair_sessions_events(db)` — pairs `Events` rows as described above. 11. `create_sessions_snapshot(db)` — `DELETE FROM Sessions; INSERT INTO Sessions SELECT * FROM Convert_Events_to_Sessions`. `Sessions` reflects step 10's pairing from here. 12. `insertOnlineHistory(db)` — dashboard graph rollup. diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index 5c7234b6..89267fc6 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -34,7 +34,7 @@ Covers what happens after a plugin's rows land in `CurrentScan`: presence comput 6. `update_presence_from_CurrentScan(db)` — sets `devPresentLastScan` from `CurrentScan` for this cycle (step 2 reads this as "previous" on the *next* cycle). 7. `update_devPresentLastScan_based_on_nics(db)` — NIC/parent-child presence aggregation; can override step 6 for parent devices. 8. `update_devPresentLastScan_based_on_force_status(db)` — the user's manual `devForceStatus` override; runs last, wins over everything above. -9. `update_vendors_from_mac`, `update_ipv4_ipv6`, `update_icons_and_types` +9. `update_vendors_from_mac`, `update_ipv4_ipv6`, `update_icons_and_types` — `update_ipv4_ipv6()` does **not** go through `LatestDeviceScan`/`FIELD_SPECS`; it reads `CurrentScan` directly with its own `PARTITION BY scanMac, address_family` ranking, so a device reporting both an IPv4 and an IPv6 row in the same cycle gets both `devPrimaryIPv4`/`devPrimaryIPv6` set from that cycle, not just whichever family happened to win `devLastIP`'s single-value reduction. See `.gemini/internal-docs/PRDs/dual-stack-primary-ip-support.md`. 10. `pair_sessions_events(db)` — pairs `Events` rows as described above. 11. `create_sessions_snapshot(db)` — `DELETE FROM Sessions; INSERT INTO Sessions SELECT * FROM Convert_Events_to_Sessions`. `Sessions` reflects step 10's pairing from here. 12. `insertOnlineHistory(db)` — dashboard graph rollup. diff --git a/server/scan/device_handling.py b/server/scan/device_handling.py index cbaaf97a..02429d9b 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -348,39 +348,51 @@ def update_devices_data_from_scan(db): def update_ipv4_ipv6(db): """ - Fill devPrimaryIPv4 and devPrimaryIPv6 based on devLastIP. - Skips empty devLastIP and preserves existing values for the other version. + Fill devPrimaryIPv4 and devPrimaryIPv6 from CurrentScan directly, ranking each + scanMac's rows independently per address family (not per plugin, not via the + single already-reduced devLastIP) so a device reporting both families in the + same scan cycle - dual-stack, the normal case on any modern network, not an + edge case - gets both fields populated from that one cycle instead of only + whichever family happened to win devLastIP's single-value reduction. + Skips empty/presence-suppressed rows and preserves existing values for a + family not refreshed this cycle. See .gemini/internal-docs/PRDs/dual-stack-primary-ip-support.md. """ sql = db.sql - mylog("debug", "[Update Devices] Updating devPrimaryIPv4 / devPrimaryIPv6 from devLastIP") + mylog("debug", "[Update Devices] Updating devPrimaryIPv4 / devPrimaryIPv6 from CurrentScan") - devices = sql.execute("SELECT devMac, devLastIP FROM Devices").fetchall() - records_to_update = [] + rows = sql.execute(f""" + WITH ranked AS ( + SELECT + scanMac, + scanLastIP, + CASE WHEN instr(scanLastIP, ':') > 0 THEN 'v6' ELSE 'v4' END AS family, + ROW_NUMBER() OVER ( + PARTITION BY scanMac, CASE WHEN instr(scanLastIP, ':') > 0 THEN 'v6' ELSE 'v4' END + ORDER BY scanLastConnection DESC + ) AS family_rn + FROM CurrentScan + WHERE scanLastIP NOT IN ({NULL_EQUIVALENTS_SQL}) + AND scanPresence = 1 + ) + SELECT scanMac, family, scanLastIP FROM ranked WHERE family_rn = 1 + """).fetchall() - for device in devices: - last_ip = device["devLastIP"] - # Keeping your specific skip logic - if not last_ip or last_ip.lower() in ("", "null", "(unknown)", "(Unknown)"): - continue - - ipv4, ipv6 = None, None + per_mac = {} + for row in rows: + mac, family, ip = row["scanMac"], row["family"], row["scanLastIP"] try: - ip_obj = ipaddress.ip_address(last_ip) - if ip_obj.version == 4: - ipv4 = last_ip - else: - ipv6 = last_ip + ipaddress.ip_address(ip) # defensive re-validation of the SQL-side family classification except ValueError: continue + per_mac.setdefault(mac, {})[family] = ip - records_to_update.append((ipv4, ipv6, device["devMac"])) + records_to_update = [ + (v.get("v4"), v.get("v6"), mac) for mac, v in per_mac.items() + ] if records_to_update: # We use COALESCE(?, Column) so that if the first arg is NULL, # it keeps the current value of the column. - - # mylog("none", f"[Update Devices] Updated records_to_update: {records_to_update}") - sql.executemany( """ UPDATE Devices diff --git a/test/scan/test_ip_format_and_locking.py b/test/scan/test_ip_format_and_locking.py index d9989a31..08693373 100644 --- a/test/scan/test_ip_format_and_locking.py +++ b/test/scan/test_ip_format_and_locking.py @@ -169,3 +169,96 @@ def test_ipv6_address_format_variations(scan_db, mock_ip_handlers): row = cur.execute("SELECT devPrimaryIPv6 FROM Devices WHERE devLastIP = ?", (ipv6,)).fetchone() assert row is not None + +# --- Dual-stack same-cycle tests (regression for GitHub #1804) --- +# +# The tests above all cover a single address family per scan cycle - either +# one row per mac, or two *separate* cycles (CurrentScan cleared between +# them). None of them reproduce #1804: a device reporting both an IPv4 and an +# IPv6 row for the same mac in the *same* cycle. See +# .gemini/internal-docs/PRDs/dual-stack-primary-ip-support.md. + + +def test_dual_stack_same_cycle_sets_both_primary_ips(scan_db, mock_ip_handlers): + """A single scan cycle reporting both IPv4 and IPv6 for one MAC, from the + same plugin, must set both devPrimaryIPv4 and devPrimaryIPv6 from that one + cycle - regression test for #1804.""" + cur = scan_db.cursor() + cur.execute("INSERT INTO Devices (devMac) VALUES (?)", ("cc:cc:cc:cc:cc:01",)) + cur.execute( + "INSERT INTO CurrentScan (scanMac, scanLastIP, scanSourcePlugin, scanLastConnection) VALUES (?, ?, ?, ?)", + ("cc:cc:cc:cc:cc:01", "192.168.1.50", "FREEBOX", "2025-01-01 01:00:00") + ) + cur.execute( + "INSERT INTO CurrentScan (scanMac, scanLastIP, scanSourcePlugin, scanLastConnection) VALUES (?, ?, ?, ?)", + ("cc:cc:cc:cc:cc:01", "fe80::abcd", "FREEBOX", "2025-01-01 01:00:01") + ) + scan_db.commit() + + db = Mock(sql_connection=scan_db, sql=cur) + device_handling.update_devices_data_from_scan(db) + device_handling.update_ipv4_ipv6(db) + + row = cur.execute( + "SELECT devPrimaryIPv4, devPrimaryIPv6 FROM Devices WHERE devMac = ?", + ("cc:cc:cc:cc:cc:01",), + ).fetchone() + assert row["devPrimaryIPv4"] == "192.168.1.50" + assert row["devPrimaryIPv6"] == "fe80::abcd" + + +def test_dual_stack_two_plugins_same_cycle_sets_both(scan_db, mock_ip_handlers): + """Same as above, but the IPv4 row and the IPv6 row come from two + different plugins - confirms the per-family ranking is mac-wide, not + scoped to one plugin's own rows.""" + cur = scan_db.cursor() + cur.execute("INSERT INTO Devices (devMac) VALUES (?)", ("cc:cc:cc:cc:cc:02",)) + cur.execute( + "INSERT INTO CurrentScan (scanMac, scanLastIP, scanSourcePlugin, scanLastConnection) VALUES (?, ?, ?, ?)", + ("cc:cc:cc:cc:cc:02", "192.168.1.60", "ARPSCAN", "2025-01-01 01:00:00") + ) + cur.execute( + "INSERT INTO CurrentScan (scanMac, scanLastIP, scanSourcePlugin, scanLastConnection) VALUES (?, ?, ?, ?)", + ("cc:cc:cc:cc:cc:02", "fe80::beef", "FREEBOX", "2025-01-01 01:00:00") + ) + scan_db.commit() + + db = Mock(sql_connection=scan_db, sql=cur) + device_handling.update_devices_data_from_scan(db) + device_handling.update_ipv4_ipv6(db) + + row = cur.execute( + "SELECT devPrimaryIPv4, devPrimaryIPv6 FROM Devices WHERE devMac = ?", + ("cc:cc:cc:cc:cc:02",), + ).fetchone() + assert row["devPrimaryIPv4"] == "192.168.1.60" + assert row["devPrimaryIPv6"] == "fe80::beef" + + +def test_dual_stack_presence_suppressed_row_excluded(scan_db, mock_ip_handlers): + """A scanPresence=0 row must not win a device's primary address for that + family, same as it doesn't count as a live sighting elsewhere in the scan + pipeline.""" + cur = scan_db.cursor() + cur.execute("INSERT INTO Devices (devMac) VALUES (?)", ("cc:cc:cc:cc:cc:03",)) + cur.execute( + "INSERT INTO CurrentScan (scanMac, scanLastIP, scanSourcePlugin, scanLastConnection, scanPresence) VALUES (?, ?, ?, ?, ?)", + ("cc:cc:cc:cc:cc:03", "192.168.1.70", "ARPSCAN", "2025-01-01 01:00:00", 1) + ) + cur.execute( + "INSERT INTO CurrentScan (scanMac, scanLastIP, scanSourcePlugin, scanLastConnection, scanPresence) VALUES (?, ?, ?, ?, ?)", + ("cc:cc:cc:cc:cc:03", "fe80::dead", "SOMEPLG", "2025-01-01 01:00:00", 0) + ) + scan_db.commit() + + db = Mock(sql_connection=scan_db, sql=cur) + device_handling.update_devices_data_from_scan(db) + device_handling.update_ipv4_ipv6(db) + + row = cur.execute( + "SELECT devPrimaryIPv4, devPrimaryIPv6 FROM Devices WHERE devMac = ?", + ("cc:cc:cc:cc:cc:03",), + ).fetchone() + assert row["devPrimaryIPv4"] == "192.168.1.70" + assert row["devPrimaryIPv6"] in (None, "") +