diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index cf1f99907..6c6775894 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -56,6 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. +8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. ## When to read this vs. other docs/skills diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index f42a09e0f..e9ece89b1 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -56,6 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. +8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. ## When to read this vs. other docs/skills diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index a8e3ef0d0..1550ec69c 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -56,6 +56,7 @@ This is the scan-pipeline-local half of a bigger attribution system — see `dat 5. **A blank/null-equivalent `scanMac` can create a phantom `Devices` row.** `create_new_devices()`'s two creation-path queries filter `scanMac NOT IN (NULL_EQUIVALENTS_SQL)` (`server/scan/device_handling.py`, `const.NULL_EQUIVALENTS_SQL`) as a backstop, because `scanCreatesDevice` defaults to `1` — any plugin reporting a row with no real MAC, without setting `scanCreatesDevice = 0` itself, would otherwise create a `devMac = ''` device, and every other blank-MAC row from every other plugin would then silently write onto it. The filter doesn't replace `scanCreatesDevice = 0` as the correct thing for a plugin to set on such rows; it keeps a MAC-less row inert when some other plugin forgets to. Check any new creation-adjacent query against blank `scanMac` too. 6. **`app.sql` is not dead code.** `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it into `sqlite3` to bootstrap a brand-new database on first install; `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift: each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same treatment inside the shared `ensure_plugins_tables()`. `AppEvents` gets its own drop/recreate via `AppEvent_obj.__init__()` (`server/workflows/app_events.py`), independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. `Events`, `Sessions`, and `Notifications` get the same backfill via `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`). `AppEvents`/`Notifications` each also have a second schema-definition surface — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` — kept in sync by the same drift-check test. Check any new query here with `EXPLAIN QUERY PLAN` at a realistic row count rather than assuming it's fine because it resembles an existing one — a correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable while actually being quadratic at this scale. 7. **`LatestEventsPerMAC` is intentionally `CurrentScan`-gated - correct for its one existing caller, a trap for a new one.** It `INNER JOIN`s `CurrentScan`, which is exactly right for the mainline "New Connections" query (every MAC it looks up is already known to be in `CurrentScan` this cycle, via its own `present_agg`). It is not a general-purpose "last event for any MAC" lookup. A caller that needs the last event for a MAC *not* in `CurrentScan` this cycle (e.g. a NIC-covered parent with no direct scan row of its own) gets no row at all from this view, regardless of that MAC's real event history - silently, not an error. `insert_events()`'s NIC-derived reconnect query reads `Events` directly instead (a correlated `ORDER BY eveDateTime DESC LIMIT 1`, covered by `idx_eve_mac_datetime_desc`), sidestepping the view entirely rather than trying to make it handle both shapes. +8. **A correlated helper's own internal subquery alias can be shadowed by an identically-named alias at the call site, silently collapsing its `EXISTS` into a table-wide tautology.** SQL scoping makes the innermost alias declaration win. `nic_derived_presence_condition()` used to alias its own inner scan as `nic_parent`; `insert_events()`'s NIC-derived reconnect query aliases its own row the same way, so `nic_derived_presence_condition("nic_parent.devMac")` silently checked "does any device in `Devices` satisfy this," not "does this one" - any NIC-covered parent anywhere in the table made every other absent parent look NIC-derived-present too. Fixed by renaming the helper's internal alias to `nic_presence_parent` and rejecting it as a `mac_column` argument, matching `current_scan_presence_condition()`'s existing `presence_scan` guard. Only caught by a test with two sibling devices in one DB where one should match and the other shouldn't - every single-device test passed regardless, because "any row" and "this row" are the same row when there's only one. ## When to read this vs. other docs/skills diff --git a/server/scan/presence.py b/server/scan/presence.py index a2b4a26ec..1837011e5 100644 --- a/server/scan/presence.py +++ b/server/scan/presence.py @@ -66,6 +66,19 @@ def nic_derived_presence_condition(mac_column: str) -> str: mac_column must be a trusted, hardcoded SQL column/table.column reference written by NetAlertX code - same constraint as current_scan_presence_condition(), enforced the same way. + + The inner Devices scan is aliased as nic_presence_parent, not the more + obvious nic_parent, for the same shadowing reason + current_scan_presence_condition() aliases its own inner CurrentScan as + presence_scan rather than bare CurrentScan: a caller correlating this + condition from a query that itself aliases its row as nic_parent (a + natural name to pick, given this helper's own docstring uses it) would + otherwise have mac_column="nic_parent.devMac" resolve to this + subquery's own inner alias instead of the caller's outer row, collapsing + the comparison into an always-true same-row tautology - confirmed live + in this exact shape in nic-parent-reconnect-events.md's implementation + notes. mac_column may not reference nic_presence_parent for the same + reason presence_scan is guarded below. """ if not _SQL_IDENTIFIER_RE.match(mac_column): raise ValueError(f"mac_column must be a plain identifier, got: {mac_column!r}") @@ -75,29 +88,34 @@ def nic_derived_presence_condition(mac_column: str) -> str: f"current_scan_presence_condition()'s own internal subquery " f"alias, got: {mac_column!r}" ) + if mac_column == "nic_presence_parent" or mac_column.startswith("nic_presence_parent."): + raise ValueError( + f"mac_column must not reference nic_presence_parent - that's " + f"this function's own internal subquery alias, got: {mac_column!r}" + ) return f"""EXISTS ( - SELECT 1 FROM Devices AS nic_parent - WHERE nic_parent.devMac = {mac_column} + SELECT 1 FROM Devices AS nic_presence_parent + WHERE nic_presence_parent.devMac = {mac_column} AND ( ( - IFNULL(CAST(nic_parent.devReqNicsOnline AS TEXT), '') = '1' + IFNULL(CAST(nic_presence_parent.devReqNicsOnline AS TEXT), '') = '1' AND EXISTS (SELECT 1 FROM Devices AS any_nic - WHERE any_nic.devParentMAC = nic_parent.devMac + WHERE any_nic.devParentMAC = nic_presence_parent.devMac AND any_nic.devParentRelType = 'nic') AND NOT EXISTS ( SELECT 1 FROM Devices AS nic - WHERE nic.devParentMAC = nic_parent.devMac + WHERE nic.devParentMAC = nic_presence_parent.devMac AND nic.devParentRelType = 'nic' AND NOT {current_scan_presence_condition("nic.devMac")} ) ) OR ( - IFNULL(CAST(nic_parent.devReqNicsOnline AS TEXT), '') != '1' + IFNULL(CAST(nic_presence_parent.devReqNicsOnline AS TEXT), '') != '1' AND EXISTS ( SELECT 1 FROM Devices AS nic - WHERE nic.devParentMAC = nic_parent.devMac + WHERE nic.devParentMAC = nic_presence_parent.devMac AND nic.devParentRelType = 'nic' AND {current_scan_presence_condition("nic.devMac")} ) diff --git a/test/scan/test_down_sleep_events.py b/test/scan/test_down_sleep_events.py index f3b0b89ad..ba06ec7cb 100644 --- a/test/scan/test_down_sleep_events.py +++ b/test/scan/test_down_sleep_events.py @@ -948,6 +948,28 @@ class TestInsertEventsNicDerivedReconnect: assert _event_types_for(conn, "aa:22:00:00:00:09") == [] + def test_sibling_parent_with_absent_nics_not_falsely_reconnected(self): + """Regression: an alias collision inside nic_derived_presence_condition() + used to collapse its per-row check into a table-wide tautology - any + NIC-covered parent anywhere in Devices made every other absent parent + look NIC-derived-present too. A single-parent-per-DB test can't catch + this (there's nothing else in the table to falsely match against), so + this puts two sibling parents in one DB on purpose.""" + conn = _make_db() + _setup_parent_with_nics( + conn, "aa:22:00:00:00:10", [("bb:22:00:00:00:10", True)], + parent_present_last_scan=0, + ) + _setup_parent_with_nics( + conn, "aa:22:00:00:00:11", [("bb:22:00:00:00:11", False)], + parent_present_last_scan=0, + ) + + insert_events(DummyDB(conn)) + + assert _event_types_for(conn, "aa:22:00:00:00:10") == ["Connected"] + assert _event_types_for(conn, "aa:22:00:00:00:11") == [] + def test_orphan_pairing_closes_end_to_end(self): """The reporter's exact scenario (issue #1821): absent -> Disconnected -> NIC-present -> Connected -> absent again -> diff --git a/test/scan/test_presence_helper.py b/test/scan/test_presence_helper.py index c16db0782..d48a9f886 100644 --- a/test/scan/test_presence_helper.py +++ b/test/scan/test_presence_helper.py @@ -75,8 +75,8 @@ class TestNicDerivedHelperCorrectness: def test_returns_expected_sql_fragment(self): result = nic_derived_presence_condition("devMac") assert "EXISTS (" in result - assert "SELECT 1 FROM Devices AS nic_parent" in result - assert "nic_parent.devMac = devMac" in result + assert "SELECT 1 FROM Devices AS nic_presence_parent" in result + assert "nic_presence_parent.devMac = devMac" in result assert "devReqNicsOnline" in result @pytest.mark.parametrize("bad_value", [ @@ -96,6 +96,14 @@ class TestNicDerivedHelperCorrectness: with pytest.raises(ValueError): nic_derived_presence_condition(bad_value) + @pytest.mark.parametrize("bad_value", ["nic_presence_parent", "nic_presence_parent.devMac"]) + def test_rejects_own_internal_alias(self, bad_value): + """A caller referencing this function's own internal alias would hit + the exact shadowing bug that motivated the nic_parent -> + nic_presence_parent rename - see this module's docstring.""" + with pytest.raises(ValueError): + nic_derived_presence_condition(bad_value) + class TestQualifiedColumnExecutesCorrectly: """Executes the fragment, not just checks the generated SQL text - proves @@ -119,6 +127,35 @@ class TestQualifiedColumnExecutesCorrectly: "into a table-wide 'does anything assert presence' check" ) + def test_nic_derived_condition_correlates_when_caller_aliases_nic_parent(self): + """Regression: nic_derived_presence_condition() used to alias its own + inner scan as nic_parent too, so a caller that (like insert_events()'s + NIC-derived reconnect query) aliases its own row as nic_parent got + mac_column="nic_parent.devMac" shadowed by the helper's own inner + alias - collapsing the EXISTS into a table-wide tautology instead of + a per-row check.""" + conn = sqlite3.connect(":memory:") + conn.execute("""CREATE TABLE Devices (devMac TEXT, devReqNicsOnline INTEGER, + devParentMAC TEXT, devParentRelType TEXT)""") + conn.execute("CREATE TABLE CurrentScan (scanMac TEXT, scanPresence INTEGER)") + conn.execute("INSERT INTO Devices VALUES ('aa', 0, NULL, NULL)") + conn.execute("INSERT INTO Devices VALUES ('aa-nic', 0, 'aa', 'nic')") + conn.execute("INSERT INTO CurrentScan VALUES ('aa-nic', 1)") # aa's NIC is present + conn.execute("INSERT INTO Devices VALUES ('bb', 0, NULL, NULL)") + conn.execute("INSERT INTO Devices VALUES ('bb-nic', 0, 'bb', 'nic')") # bb's NIC absent + conn.commit() + + condition = nic_derived_presence_condition("nic_parent.devMac") + rows = conn.execute( + f"""SELECT devMac, {condition} AS is_nic_present FROM Devices AS nic_parent + WHERE devParentMAC IS NULL""" + ).fetchall() + + assert dict(rows) == {"aa": 1, "bb": 0}, ( + "each outer row must be checked against its own NIC children, not " + "collapse into a table-wide 'does any device have NIC presence' check" + ) + def _call_count(func, target_name="current_scan_presence_condition"): """Count calls to target_name within func's own source (AST-based, not