diff --git a/.claude/skills/pr-analysis/SKILL.md b/.claude/skills/pr-analysis/SKILL.md index 53ce8bb78..3a17b8a88 100644 --- a/.claude/skills/pr-analysis/SKILL.md +++ b/.claude/skills/pr-analysis/SKILL.md @@ -28,6 +28,10 @@ Run through this before creating or editing any file under `test/`: 2. Load the `testing-workflow` skill — any test additions or changes must follow it. 3. Load any domain-specific skill relevant to the files being changed (e.g. `database-patterns` for DB writes, `settings-management` for config). +## Verifying a Claim About Generated Code + +A finding that claims a specific SQL/code expansion result (an alias collision, a macro substitution, an interpolation outcome) can't be verified by checking the caller's own internal consistency alone - the caller can be perfectly self-consistent and still collide with something the callee does internally that isn't visible at the call site. Open and read the callee's actual definition before accepting or rejecting the claim, and if it's a runtime-behavior claim (not just syntax), run a minimal repro to confirm rather than reasoning about it in the abstract. A real case: a finding claimed two same-named aliases collided across a caller/helper boundary; checking only that the caller used its alias consistently looked like it disproved the finding, but the helper had its own same-named internal alias that was never inspected - the finding was correct. + ## Comment Classification For each comment, determine: diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index 6c6775894..06e48bdc1 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -56,7 +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. +8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. 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/pr-analysis/SKILL.md b/.gemini/skills/pr-analysis/SKILL.md index 5d3d3417e..88442278a 100644 --- a/.gemini/skills/pr-analysis/SKILL.md +++ b/.gemini/skills/pr-analysis/SKILL.md @@ -28,6 +28,10 @@ Run through this before creating or editing any file under `test/`: 2. Load `testing-workflow` skill — any test additions or changes must follow it. 3. Load any domain-specific skill relevant to the files being changed (e.g. `database-patterns` for DB writes, `settings` for config). +## Verifying a Claim About Generated Code + +A finding that claims a specific SQL/code expansion result (an alias collision, a macro substitution, an interpolation outcome) can't be verified by checking the caller's own internal consistency alone - the caller can be perfectly self-consistent and still collide with something the callee does internally that isn't visible at the call site. Open and read the callee's actual definition before accepting or rejecting the claim, and if it's a runtime-behavior claim (not just syntax), run a minimal repro to confirm rather than reasoning about it in the abstract. A real case: a finding claimed two same-named aliases collided across a caller/helper boundary; checking only that the caller used its alias consistently looked like it disproved the finding, but the helper had its own same-named internal alias that was never inspected - the finding was correct. + ## Comment Classification For each comment, determine: diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index e9ece89b1..224a5e834 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -56,7 +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. +8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. 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/pr-analysis/SKILL.md b/.github/skills/pr-analysis/SKILL.md index 79f0bef10..6251ae55d 100644 --- a/.github/skills/pr-analysis/SKILL.md +++ b/.github/skills/pr-analysis/SKILL.md @@ -28,6 +28,10 @@ Run through this before creating or editing any file under `test/`: 2. Load `testing-workflow` skill — any test additions or changes must follow it. 3. Load any domain-specific skill relevant to the files being changed (e.g. `database-patterns` for DB writes, `settings-management` for config). +## Verifying a Claim About Generated Code + +A finding that claims a specific SQL/code expansion result (an alias collision, a macro substitution, an interpolation outcome) can't be verified by checking the caller's own internal consistency alone - the caller can be perfectly self-consistent and still collide with something the callee does internally that isn't visible at the call site. Open and read the callee's actual definition before accepting or rejecting the claim, and if it's a runtime-behavior claim (not just syntax), run a minimal repro to confirm rather than reasoning about it in the abstract. A real case: a finding claimed two same-named aliases collided across a caller/helper boundary; checking only that the caller used its alias consistently looked like it disproved the finding, but the helper had its own same-named internal alias that was never inspected - the finding was correct. + ## Comment Classification For each comment, determine: diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index 1550ec69c..344549977 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -56,7 +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. +8. **A correlated helper's `mac_column` argument silently binds to the helper's own inner row, not the caller's, whenever the helper's inner table already has a column of that name in scope - alias or no alias.** SQL resolves an unqualified name in the innermost enclosing scope first and only searches outward if nothing matches there; `nic_derived_presence_condition()`'s inner scan is `FROM Devices`, and `Devices` has a `devMac` column, so *any* bare `"devMac"` argument - not just one that happens to collide with an alias name - bound to the helper's own inner row. Every bare-`"devMac"` call site (both `Device Down` queries, `Disconnected`, `update_devLastConnection_from_CurrentScan()`) was affected: any NIC-covered device anywhere in `Devices` made every *other* absent device, including one with no NIC children at all, look NIC-derived-present, silently suppressing its real event or bumping its `devLastConnection`. (A qualified-but-colliding argument, e.g. `"nic_parent.devMac"` passed from a caller aliasing its own row `nic_parent` while the helper's own inner alias was also `nic_parent`, is the same root cause in a narrower form.) Fixed by requiring every caller to pass a qualified reference to *its own* table/alias (`"Devices.devMac"`, `"DevicesView.devMac"`) and having `nic_derived_presence_condition()` reject a bare `mac_column` outright, on top of the existing `presence_scan`/`nic_presence_parent`-collision guards. `current_scan_presence_condition()` doesn't need this: its inner scan is `FROM CurrentScan`, which has no `devMac` column, so a bare `"devMac"` has nothing to bind to inward and correctly falls back to the caller's row. 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/device_handling.py b/server/scan/device_handling.py index 1ac548bdc..d12cde688 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -242,7 +242,7 @@ def update_devLastConnection_from_CurrentScan(db): UPDATE Devices SET devLastConnection = '{startTime}' WHERE ({current_scan_presence_condition("devMac")} - OR {nic_derived_presence_condition("devMac")}) + OR {nic_derived_presence_condition("Devices.devMac")}) """) diff --git a/server/scan/presence.py b/server/scan/presence.py index 1837011e5..7a7c67341 100644 --- a/server/scan/presence.py +++ b/server/scan/presence.py @@ -75,13 +75,35 @@ def nic_derived_presence_condition(mac_column: str) -> str: 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. + the comparison into an always-true same-row tautology. mac_column may + not reference nic_presence_parent for the same reason presence_scan is + guarded below. + + mac_column MUST be qualified (e.g. "Devices.devMac", "DevicesView.devMac" + - never bare "devMac"), unlike current_scan_presence_condition() where a + bare column is fine. The reason is column, not alias, shadowing: this + function's own inner scan is `FROM Devices`, and Devices has a devMac + column, so an unqualified devMac in the substituted WHERE always + resolves to *this* function's own inner row - SQL prefers the innermost + enclosing scope for an unqualified name and only searches outward if the + inner scope has no matching column, so it never even reaches the + caller's outer row. (current_scan_presence_condition()'s inner scan is + `FROM CurrentScan`, which has no devMac column, so a bare "devMac" there + has nothing to bind to inward and correctly falls back outward.) + Confirmed live: an unqualified caller made every device with no NIC + children of its own read as NIC-derived-present, as soon as *any* other + device anywhere in Devices legitimately had one. """ if not _SQL_IDENTIFIER_RE.match(mac_column): raise ValueError(f"mac_column must be a plain identifier, got: {mac_column!r}") + if "." not in mac_column: + raise ValueError( + f"mac_column must be qualified with the caller's own table/alias " + f"(e.g. 'Devices.devMac', not bare 'devMac') - Devices (this " + f"function's own inner scan) already has a devMac column, so an " + f"unqualified reference always binds to this function's own inner " + f"row instead of the caller's, got: {mac_column!r}" + ) if mac_column == "presence_scan" or mac_column.startswith("presence_scan."): raise ValueError( f"mac_column must not reference presence_scan - that's " diff --git a/server/scan/session_events.py b/server/scan/session_events.py index 8769d8c51..66373705c 100755 --- a/server/scan/session_events.py +++ b/server/scan/session_events.py @@ -210,7 +210,7 @@ def insert_events(db): AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} AND NOT ({current_scan_presence_condition("devMac")} - OR {nic_derived_presence_condition("devMac")}) """) + OR {nic_derived_presence_condition("DevicesView.devMac")}) """) # Check device down – sleeping devices whose sleep window has expired mylog("debug", "[Events] - 1b - Devices down (sleep expired)") @@ -225,7 +225,7 @@ def insert_events(db): AND devPresentLastScan = 0 AND {_SQL_NOT_FORCED_ONLINE} AND NOT ({current_scan_presence_condition("devMac")} - OR {nic_derived_presence_condition("devMac")}) + OR {nic_derived_presence_condition("DevicesView.devMac")}) AND NOT EXISTS (SELECT 1 FROM Events WHERE eveMac = devMac AND eveEventType = 'Device Down' @@ -323,7 +323,7 @@ def insert_events(db): AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} AND NOT ({current_scan_presence_condition("devMac")} - OR {nic_derived_presence_condition("devMac")}) """) + OR {nic_derived_presence_condition("Devices.devMac")}) """) # Check IP Changed mylog("debug", "[Events] - 4 - IP Changes") diff --git a/test/scan/test_down_sleep_events.py b/test/scan/test_down_sleep_events.py index ba06ec7cb..f5a94b5d1 100644 --- a/test/scan/test_down_sleep_events.py +++ b/test/scan/test_down_sleep_events.py @@ -650,6 +650,31 @@ class TestInsertEventsNicDerivedPresence: assert "aa:11:00:00:00:03" in _down_event_macs(conn.cursor()) + def test_unrelated_device_without_nics_not_falsely_suppressed(self): + """Regression: an unqualified mac_column in + nic_derived_presence_condition() used to bind to the function's own + inner Devices row instead of the caller's, so any device anywhere + with NIC-derived presence made every OTHER absent device (even one + with no NIC children at all) look NIC-derived-present too, silently + suppressing its real 'Device Down' event.""" + conn = _make_db() + _setup_parent_with_nics(conn, "aa:11:00:00:00:11", [("bb:11:00:00:00:11", True)]) + cur = conn.cursor() + _insert_device(cur, "cc:11:00:00:00:11", alert_down=1, present_last_scan=1) + conn.commit() + + insert_events(DummyDB(conn)) + + down_macs = _down_event_macs(conn.cursor()) + assert "aa:11:00:00:00:11" not in down_macs, ( + "the actual NIC-covered parent must still be suppressed" + ) + assert "cc:11:00:00:00:11" in down_macs, ( + "a device with no NIC children of its own must still get its real " + "'Device Down' event, not be suppressed just because an unrelated " + "device elsewhere has NIC-derived presence" + ) + def test_all_mode_one_nic_missing_still_fires(self): """Mirrors test_req_all_mode_partial_nics_does_not_raise_absent_parent in test_nic_presence.py - same ANY/ALL contract, different pipeline point.""" diff --git a/test/scan/test_presence_helper.py b/test/scan/test_presence_helper.py index d48a9f886..7b41c5733 100644 --- a/test/scan/test_presence_helper.py +++ b/test/scan/test_presence_helper.py @@ -73,12 +73,21 @@ class TestNicDerivedHelperCorrectness: PRD Design §1. Same trust-boundary checks as current_scan_presence_condition().""" def test_returns_expected_sql_fragment(self): - result = nic_derived_presence_condition("devMac") + result = nic_derived_presence_condition("Devices.devMac") assert "EXISTS (" in result assert "SELECT 1 FROM Devices AS nic_presence_parent" in result - assert "nic_presence_parent.devMac = devMac" in result + assert "nic_presence_parent.devMac = Devices.devMac" in result assert "devReqNicsOnline" in result + @pytest.mark.parametrize("bad_value", ["devMac", "scanMac"]) + def test_rejects_unqualified_column(self, bad_value): + """Devices (this function's own inner scan) has a devMac column, so a + bare mac_column always binds to the function's own inner row instead + of the caller's - see this module's docstring for the confirmed-live + failure mode this guards against.""" + with pytest.raises(ValueError): + nic_derived_presence_condition(bad_value) + @pytest.mark.parametrize("bad_value", [ "devMac; DROP TABLE Devices--", "devMac OR 1=1", diff --git a/test/scan/test_scan_presence.py b/test/scan/test_scan_presence.py index cd64f3fdb..d915d0b21 100644 --- a/test/scan/test_scan_presence.py +++ b/test/scan/test_scan_presence.py @@ -155,6 +155,52 @@ class TestDevLastConnectionRespectsPresence: ).fetchone() assert row["devLastConnection"] != "2020-01-01 00:00:00" + def test_unrelated_device_without_nics_not_falsely_bumped(self): + """Regression: nic_derived_presence_condition() used to be callable + with a bare, unqualified mac_column - Devices (this function's own + inner scan) has a devMac column, so the unqualified reference always + bound to the function's own inner row instead of the caller's, + making every device with no NIC children of its own read as + NIC-derived-present as soon as *any other* device anywhere in + Devices legitimately had one. Two devices in one DB on purpose - a + single-device DB can't distinguish "per-row correct" from + "table-wide tautology".""" + conn = make_db() + parent_mac = "aa:22:00:00:00:03" + nic_mac = "bb:22:00:00:00:03" + unrelated_mac = "cc:22:00:00:00:03" + insert_device_from_dict(conn, make_device_dict( + parent_mac, devLastConnection="2020-01-01 00:00:00", + devParentMAC="", devParentRelType="", devReqNicsOnline=0, + )) + insert_device_from_dict(conn, make_device_dict( + nic_mac, devParentMAC=parent_mac, devParentRelType="nic", devReqNicsOnline=0, + )) + insert_current_scan_row_from_dict( + conn, make_current_scan_dict(nic_mac, scanPresence=1) + ) + insert_device_from_dict(conn, make_device_dict( + unrelated_mac, devLastConnection="2020-01-01 00:00:00", + devParentMAC="", devParentRelType="", devReqNicsOnline=0, + )) + db = DummyDB(conn) + + device_handling.update_devLastConnection_from_CurrentScan(db) + + parent_row = conn.execute( + "SELECT devLastConnection FROM Devices WHERE devMac = ?", (parent_mac,) + ).fetchone() + unrelated_row = conn.execute( + "SELECT devLastConnection FROM Devices WHERE devMac = ?", (unrelated_mac,) + ).fetchone() + assert parent_row["devLastConnection"] != "2020-01-01 00:00:00", ( + "the actual NIC-covered parent must still bump" + ) + assert unrelated_row["devLastConnection"] == "2020-01-01 00:00:00", ( + "a device with no NIC children of its own must not be bumped just " + "because some other device in Devices has NIC-derived presence" + ) + class TestNewConnectionsRespectsPresence: """insert_events()'s New Connections query must not fire Connected for a