diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index ab0eff0e1..9781183ae 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -47,13 +47,14 @@ This skill covers what happens *after* a plugin's rows land in `CurrentScan` — This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. -## Two real gotchas (not hypothetical — both surfaced live during a design review) +## Four real gotchas (not hypothetical — all surfaced live during a design review) -1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. +1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. 3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. +4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. -**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan` specifically is safe from drift because `ensure_CurrentScan()` unconditionally drops and recreates it on every startup, superseding whatever `app.sql` bootstrapped — but that safety net is unique to the four tables with an `ensure_X()`-style function (`CurrentScan`, `Parameters`, `Settings`, `Plugins_Language_Strings`). `Events`, `Sessions`, `AppEvents`, and `Notifications` have no such function and no `ensure_column()` backfill calls in `server/database.py` either (unlike `Devices`, which has ~30 of them) — for those tables, whatever `app.sql` says *is* the schema, permanently, for every fresh install. Drift there would be a real, live bug, not documentation lag — see `.gemini/internal-docs/PRDs/scan-pipeline-hardening.md` for the follow-up this motivated. Any *new* query added here should be checked the same way (`EXPLAIN QUERY PLAN` at a realistic row count) rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. +**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, 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`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. ## 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 65352942d..a2bab0f92 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -47,13 +47,14 @@ This skill covers what happens *after* a plugin's rows land in `CurrentScan` — This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. -## Two real gotchas (not hypothetical — both surfaced live during a design review) +## Four real gotchas (not hypothetical — all surfaced live during a design review) -1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. +1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. 3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. +4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. -**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan` specifically is safe from drift because `ensure_CurrentScan()` unconditionally drops and recreates it on every startup, superseding whatever `app.sql` bootstrapped — but that safety net is unique to the four tables with an `ensure_X()`-style function (`CurrentScan`, `Parameters`, `Settings`, `Plugins_Language_Strings`). `Events`, `Sessions`, `AppEvents`, and `Notifications` have no such function and no `ensure_column()` backfill calls in `server/database.py` either (unlike `Devices`, which has ~30 of them) — for those tables, whatever `app.sql` says *is* the schema, permanently, for every fresh install. Drift there would be a real, live bug, not documentation lag — see `.gemini/internal-docs/PRDs/scan-pipeline-hardening.md` for the follow-up this motivated. Any *new* query added here should be checked the same way (`EXPLAIN QUERY PLAN` at a realistic row count) rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. +**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, 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`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. ## 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 6e455886c..e8ba8d754 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -47,13 +47,14 @@ This skill covers what happens *after* a plugin's rows land in `CurrentScan` — This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. -## Two real gotchas (not hypothetical — both surfaced live during a design review) +## Four real gotchas (not hypothetical — all surfaced live during a design review) -1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. +1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. 3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. +4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. -**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan` specifically is safe from drift because `ensure_CurrentScan()` unconditionally drops and recreates it on every startup, superseding whatever `app.sql` bootstrapped — but that safety net is unique to the four tables with an `ensure_X()`-style function (`CurrentScan`, `Parameters`, `Settings`, `Plugins_Language_Strings`). `Events`, `Sessions`, `AppEvents`, and `Notifications` have no such function and no `ensure_column()` backfill calls in `server/database.py` either (unlike `Devices`, which has ~30 of them) — for those tables, whatever `app.sql` says *is* the schema, permanently, for every fresh install. Drift there would be a real, live bug, not documentation lag — see `.gemini/internal-docs/PRDs/scan-pipeline-hardening.md` for the follow-up this motivated. Any *new* query added here should be checked the same way (`EXPLAIN QUERY PLAN` at a realistic row count) rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. +**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, 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`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. ## When to read this vs. other docs/skills diff --git a/docs/NOTIFICATIONS.md b/docs/NOTIFICATIONS.md index 7c992e83d..620fc3282 100755 --- a/docs/NOTIFICATIONS.md +++ b/docs/NOTIFICATIONS.md @@ -40,6 +40,10 @@ On almost all plugins there are 2 core settings, `_WATCH` and `_ Click the **Read more in the docs.** Link at the top of each plugin to get more details on how the given plugin works. +### Plugin-level per-row overrides + +A plugin author can also mark individual rows it reports as `quiet` via the `scanNotificationMode` data column, independent of any user-facing setting above - e.g. a bulk inventory import that shouldn't spam notifications for known-offline devices. This is a plugin-authoring concept, not something configured in the UI - see [Data contract](https://docs.netalertx.com/PLUGINS_DEV_DATA_CONTRACT#import-behavior-columns) for the full behavior (when it applies, and how it combines with the **Alert Events**/**Alert Down** device settings above when multiple plugins report the same device). + ## Global settings ⚙ ![Global notification settings](./img/NOTIFICATIONS/Global-notification-settings.png) diff --git a/docs/PLUGINS_DEV_DATA_CONTRACT.md b/docs/PLUGINS_DEV_DATA_CONTRACT.md index 7493e1b8c..50420b45f 100644 --- a/docs/PLUGINS_DEV_DATA_CONTRACT.md +++ b/docs/PLUGINS_DEV_DATA_CONTRACT.md @@ -164,7 +164,7 @@ Three optional `CurrentScan` columns, all independent of each other, control wha | Column | Type | Default | Meaning | |---|---|---|---| | `scanCreatesDevice` | boolean | `1` | Whether this row can originate a *new* `Devices` entry. `0` lets an enrich-only plugin (e.g. a hostname resolver) update an already-existing device's fields without ever being able to create one. | -| `scanNotificationMode` | text (`normal` \| `quiet`) | `normal` | Whether creating/reconnecting this device should dispatch a notification. `quiet` still writes the `Events` row (audit trail intact) but suppresses the outbound email/push. Creation-time-only: it seeds `devAlertDown`/`devAlertEvents` to `0` on the device when it's first created, rather than being an ongoing, per-cycle re-evaluated policy — reclassifying a plugin's row later does not retroactively change an already-created device's alert settings. | +| `scanNotificationMode` | text (`normal` \| `quiet`) | `normal` | Whether this row's notifications are suppressed. `quiet` always suppresses the outbound email/push; whether the `Events` row itself still gets written depends on the event. **Live** (per-cycle aggregate, reclassifying a row changes future events): `New Device`, `Connected`, `Down Reconnected`, `IP Changed` — audit trail always written. `New Device` isn't gated on `scanPresence = 1` like the other three (see flowcharts below). **Frozen** (`devAlertDown`/`devAlertEvents` seeded at device creation, reclassifying later has no retroactive effect): `Device Down`, `Disconnected` — not symmetric. `Disconnected` always writes its `Events` row (`evePendingAlertEmail = 0` when quiet). `Device Down` writes **no row at all** when `devAlertDown = 0`. | | `scanPresence` | boolean | `1` | Whether this row asserts the device is *currently online*. `0` means "identity/inventory data, no presence claim" — not "offline". A reservation, a lease record, or a static IPAM entry are typical `0` cases. | **Missing vs. invalid values — these behave differently, not interchangeably:** @@ -188,6 +188,42 @@ Three optional `CurrentScan` columns, all independent of each other, control wha `scanNotificationMode` is orthogonal to both of the above and can be combined with any row in the table (e.g. inventory import + quiet, for a fully silent bulk import of known-offline devices). +**Decision: does this row create a device?** + +```mermaid +flowchart TD + A[Row reaches CurrentScan] --> B{scanMac blank or
null-equivalent?} + B -- yes --> Z[Never creates a device] + B -- no --> C{Any row this cycle for this
MAC has scanCreatesDevice = 1?} + C -- no, all say 0 --> Y[No device created
enrich-only] + C -- yes, at least one --> D{Devices row already
exists for this MAC?} + D -- yes --> E[No-op - existing device untouched
by this check] + D -- no --> F[New Devices row created
+ New Device event] +``` + +**Decision: is this event's notification suppressed?** + +```mermaid +flowchart TD + A[Event about to fire] --> B{Fired from a row that exists in
CurrentScan this cycle? New Device /
Connected / Down Reconnected / IP Changed} + B -- yes --> C{Live aggregate: any CurrentScan row
for this MAC says
scanNotificationMode = quiet?} + C -- yes --> S[Suppressed
evePendingAlertEmail = 0] + C -- no --> N[Notified
evePendingAlertEmail = 1] + B -- no, fired from row ABSENCE
Device Down / Disconnected --> D{Frozen device setting:
devAlertDown / devAlertEvents,
seeded at creation time} + D -- off --> S + D -- on --> N +``` + +**Worked scenarios:** + +| Scenario | `scanCreatesDevice` | `scanPresence` | `scanNotificationMode` | `scanMac` | Outcome | +|---|---|---|---|---|---| +| Normal discovery (default plugin behavior) | `1` (default) | `1` (default) | `normal` (default) | real MAC | Device created if new, notified normally, presence tracked live. | +| Enrich-only plugin (e.g. a hostname resolver) | `0` | `1` (default) | `normal` (default) | real MAC | Never originates a device; still updates an existing device's fields via `FIELD_SPECS`. If another plugin reports the same MAC with `scanCreatesDevice = 1`, the device still gets created (most-permissive-wins) — this plugin's `0` doesn't block it. | +| Bulk inventory import of known-offline devices | `1` | `0` | `quiet` | real MAC | Creates devices without claiming they're online, and without a wave of "New Device" notifications for a large batch import. | +| Presence-confirming enrichment (e.g. a DHCP lease scanner) | `0` | `1` | `normal` | real MAC | Confirms an *existing* device is online without ever being the plugin that creates it. | +| Row with no usable device identity (e.g. an object with no routable MAC available) | `0` | irrelevant | irrelevant | blank / null-equivalent | Never creates a device — but not for symmetric reasons. The blank-MAC guard blocks the whole aggregated group by its shared `scanMac` value, regardless of any individual row's `scanCreatesDevice` (even a stray `1` from an unrelated plugin sharing the same blank `scanMac` can't override it). Setting `scanCreatesDevice = 0` here is still correct practice, but on its own is only this row's vote — most-permissive-wins means a sibling row for the same `scanMac` asserting `1` would still win. The blank-MAC guard is what actually guarantees safety regardless of what other contributors do. | + ## Examples ### Valid Data (9 columns, minimal) diff --git a/server/database.py b/server/database.py index 4e63ad435..f552ac2d2 100755 --- a/server/database.py +++ b/server/database.py @@ -10,6 +10,7 @@ from workflows.app_events import AppEvent_obj from db.db_upgrade import ( ensure_column, + ensure_table_columns, ensure_CurrentScan, ensure_plugins_tables, ensure_Parameters, @@ -202,6 +203,23 @@ def initDB(self): # before ensure_plugins_tables which uses IF NOT EXISTS with new names) migrate_to_camelcase(self.sql) + # Backfill Events/Sessions/Notifications columns that may be + # missing from an older app.sql snapshot - same drift-repair + # pattern as the Devices columns above, generalized instead of + # duplicated (see scan-pipeline-hardening.md Design §3). Must run + # after the camelCase migration above, or a pre-migration table's + # old-style column names would make every new-style column look + # "missing" and get added alongside the stale ones. + # + # AppEvents is deliberately NOT in this list: AppEvent_obj(self) + # below unconditionally drops and recreates it on every startup + # (its own equivalent of ensure_CurrentScan()'s drop/recreate + # pattern), so backfilling it here would be wasted work on a + # table about to be discarded a few lines later. + for _drift_table in ("Events", "Sessions", "Notifications"): + if not ensure_table_columns(self.sql, _drift_table): + raise RuntimeError(f"ensure_table_columns({_drift_table}) failed") + # Settings table setup ensure_Settings(self.sql) diff --git a/server/db/db_upgrade.py b/server/db/db_upgrade.py index 245c17597..1037ee14e 100755 --- a/server/db/db_upgrade.py +++ b/server/db/db_upgrade.py @@ -3,6 +3,7 @@ import datetime as dt from logger import mylog # noqa: E402 [flake8 lint suppression] from messaging.in_app import write_notification # noqa: E402 [flake8 lint suppression] +from db.schema_columns import TABLE_COLUMNS # noqa: E402 [flake8 lint suppression] # Define the expected Devices table columns (hardcoded base schema) [v26.1/2.XX] @@ -81,7 +82,7 @@ def ensure_column(sql, table: str, column_name: str, column_type: str) -> bool: return True # Already exists # Validate that this column is in the expected schema - expected = EXPECTED_DEVICES_COLUMNS if table == "Devices" else [] + expected = EXPECTED_DEVICES_COLUMNS if table == "Devices" else TABLE_COLUMNS.get(table, {}) if not expected or column_name not in expected: msg = ( f"[db_upgrade] ⚠ ERROR: Column '{column_name}' is not in expected schema - " @@ -102,6 +103,37 @@ def ensure_column(sql, table: str, column_name: str, column_type: str) -> bool: return False +def ensure_table_columns(sql, table: str) -> bool: + """ + Backfill every column in TABLE_COLUMNS[table] (server/db/schema_columns.py) + that's missing from the live table, via ensure_column() - the same + drift-repair pattern Devices already has, generalized instead of + hand-writing one ensure_column() call per column per table (the mirrored + duplication that pattern would otherwise reintroduce - see + scan-pipeline-hardening.md Design §3). + + Skips silently (returns True) if the table doesn't exist yet - these + four tables are normally created by app.sql's first-run bootstrap before + this ever runs, but this must not turn a not-yet-created table into a + hard failure that rolls back the whole initDB() transaction. + """ + columns = TABLE_COLUMNS.get(table) + if not columns: + mylog("none", [f"[db_upgrade] ensure_table_columns: no column list registered for '{table}'"]) + return False + + sql.execute("SELECT name FROM sqlite_master WHERE type='table' AND name=?", (table,)) + if not sql.fetchone(): + mylog("debug", [f"[db_upgrade] ensure_table_columns: '{table}' does not exist yet, skipping"]) + return True + + ok = True + for column_name, column_type in columns.items(): + if not ensure_column(sql, table, column_name, column_type): + ok = False + return ok + + def ensure_mac_lowercase_triggers(sql): """ Ensures the triggers for lowercasing MAC addresses exist on the Devices table. diff --git a/server/db/schema_columns.py b/server/db/schema_columns.py new file mode 100644 index 000000000..5371b870e --- /dev/null +++ b/server/db/schema_columns.py @@ -0,0 +1,94 @@ +""" +Single source of truth for the Events/Sessions/AppEvents/Notifications column +lists - used by both the runtime backfill loop (db_upgrade.ensure_table_columns()) +and the CI drift-check test (test/db/test_schema_drift_guard.py) against +server/db/schema/app.sql, per scan-pipeline-hardening.md Design §3. + +Unlike Devices (server/database.py's ~18 ensure_column() calls, unrelated to +this file), Events/Sessions/Notifications have no drop/recreate safety net +and previously had zero ensure_column() calls at all - app.sql was the only +definition of their schema, with nothing to catch it drifting from what the +rest of the code expects. This is additive/detection-and-backfill only; it +doesn't change any column's meaning or add new columns beyond what app.sql +already defines today. + +AppEvents is included here too (for the drift-check test's benefit, since +app.sql also defines it and workflows/app_events.py's inline CREATE TABLE is +a second definition worth keeping in sync), but is NOT part of the runtime +backfill loop in database.py's initDB() - AppEvent_obj.__init__() already +unconditionally drops and recreates the table on every startup, making its +drift harmless the same way ensure_CurrentScan() does for CurrentScan. Found +this while implementing the backfill loop, not before - the correction is +recorded in scan-pipeline-hardening.md. + +Column types are copied verbatim from server/db/schema/app.sql's CREATE +TABLE statements for these four tables - keep in sync if that file changes, +the drift-check test will fail loudly if it doesn't. +""" + +EVENTS_COLUMNS = { + "eveMac": "STRING (50)", + "eveIp": "STRING (50)", + "eveDateTime": "DATETIME", + "eveEventType": "STRING (30)", + "eveAdditionalInfo": "STRING (250)", + "evePendingAlertEmail": "BOOLEAN", + "evePairEventRowid": "INTEGER", +} + +SESSIONS_COLUMNS = { + "sesMac": "STRING (50)", + "sesIp": "STRING (50)", + "sesEventTypeConnection": "STRING (30)", + "sesDateTimeConnection": "DATETIME", + "sesEventTypeDisconnection": "STRING (30)", + "sesDateTimeDisconnection": "DATETIME", + "sesStillConnected": "BOOLEAN", + "sesAdditionalInfo": "STRING (250)", +} + +APPEVENTS_COLUMNS = { + "index": "INTEGER", + "guid": "TEXT", + "appEventProcessed": "BOOLEAN", + "dateTimeCreated": "TEXT", + "objectType": "TEXT", + "objectGuid": "TEXT", + "objectPlugin": "TEXT", + "objectPrimaryId": "TEXT", + "objectSecondaryId": "TEXT", + "objectForeignKey": "TEXT", + "objectIndex": "TEXT", + "objectIsNew": "BOOLEAN", + "objectIsArchived": "BOOLEAN", + "objectStatusColumn": "TEXT", + "objectStatus": "TEXT", + "appEventType": "TEXT", + "helper1": "TEXT", + "helper2": "TEXT", + "helper3": "TEXT", + "extra": "TEXT", +} + +NOTIFICATIONS_COLUMNS = { + "index": "INTEGER", + "guid": "TEXT", + "dateTimeCreated": "TEXT", + "dateTimePushed": "TEXT", + "status": "TEXT", + "json": "TEXT", + "text": "TEXT", + "html": "TEXT", + "publishedVia": "TEXT", + "extra": "TEXT", +} + +# Table name -> {column_name: sql_type}. Drives both the backfill loop and +# the drift-detection test - the one place these four tables' expected +# column lists are written down in Python. +TABLE_COLUMNS = { + "Events": EVENTS_COLUMNS, + "Sessions": SESSIONS_COLUMNS, + "AppEvents": APPEVENTS_COLUMNS, + "Notifications": NOTIFICATIONS_COLUMNS, +} diff --git a/server/plugins/sync/sync.py b/server/plugins/sync/sync.py index bce085a7a..813743dc7 100755 --- a/server/plugins/sync/sync.py +++ b/server/plugins/sync/sync.py @@ -307,6 +307,8 @@ def main(): else: # Fire "New Device" events for genuinely new MACs before the Devices # INSERT pre-seeds the table (which would block create_new_devices()). + # This bypasses the standard scan pipeline on purpose - see the + # scan-pipeline skill's sync.py bypass gotcha. if new_devices: now = timeNowUTC() cursor.executemany( @@ -341,7 +343,9 @@ def main(): # that contract by leaving devPresentLastScan to the normal pipeline. # NOTE: this raw SQL bypasses can_overwrite_field() — ALL other fields # including USER/LOCKED-sourced ones are overwritten. Node is fully - # authoritative in this mode. + # authoritative in this mode. Also bypasses the standard scan + # pipeline on purpose - see the scan-pipeline skill's sync.py + # bypass gotcha. _CARBON_COPY_SKIP = {'devMac', 'devPresentLastScan'} update_cols = [col for col in insert_cols if col not in _CARBON_COPY_SKIP] update_clause = ', '.join(f'{col}=excluded.{col}' for col in update_cols) diff --git a/server/scan/device_handling.py b/server/scan/device_handling.py index 84ea18173..cbaaf97a7 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -9,6 +9,7 @@ from models.device_instance import DeviceInstance from scan.name_resolution import NameResolver from scan.device_heuristics import guess_icon, guess_type +from scan.presence import current_scan_presence_condition 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 ( @@ -206,26 +207,18 @@ def update_presence_from_CurrentScan(db): # (scanPresence = 1). A row can exist purely as identity/inventory data # (scanPresence = 0, e.g. a DHCP reservation) without claiming the device is # online right now - "abstain, not override": any other row for the same MAC - # that does assert presence still wins via this same EXISTS check. - sql.execute(""" + # that does assert presence still wins via this same predicate. + sql.execute(f""" UPDATE Devices SET devPresentLastScan = 1 - WHERE EXISTS ( - SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) + WHERE {current_scan_presence_condition("devMac")} """) # Mark not present if no CurrentScan row for this MAC asserts presence - sql.execute(""" + sql.execute(f""" UPDATE Devices SET devPresentLastScan = 0 - WHERE NOT EXISTS ( - SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) + WHERE NOT {current_scan_presence_condition("devMac")} """) @@ -245,11 +238,7 @@ def update_devLastConnection_from_CurrentScan(db): sql.execute(f""" UPDATE Devices SET devLastConnection = '{startTime}' - WHERE EXISTS ( - SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) + WHERE {current_scan_presence_condition("devMac")} """) @@ -682,6 +671,7 @@ def create_new_devices(db): GROUP BY scanMac ) agg WHERE agg.scanCreates = 1 + AND agg.scanMac NOT IN ({NULL_EQUIVALENTS_SQL}) AND NOT EXISTS ( SELECT 1 FROM Devices WHERE devMac = agg.scanMac @@ -802,8 +792,12 @@ def create_new_devices(db): # if another row for the same MAC says 0 (enrich-only). Rows for # already-existing devices pass through harmlessly too - the INSERT OR # IGNORE below is already a no-op for them regardless of this filter. - query = """SELECT scanMac, scanName, scanVendor, scanSourcePlugin, scanLastIP, scanSyncHubNode, scanParentMAC, scanParentPort, scanSite, scanSSID, scanType - FROM CurrentScan WHERE scanCreatesDevice = 1""" + # scanMac NOT IN NULL_EQUIVALENTS blocks creating a device from a blank/ + # null-equivalent MAC - a plugin reporting a row it can't originate a + # device from (no real MAC available) should set scanCreatesDevice = 0 + # itself, but this is the backstop for one that doesn't. + query = f"""SELECT scanMac, scanName, scanVendor, scanSourcePlugin, scanLastIP, scanSyncHubNode, scanParentMAC, scanParentPort, scanSite, scanSSID, scanType + FROM CurrentScan WHERE scanCreatesDevice = 1 AND scanMac NOT IN ({NULL_EQUIVALENTS_SQL})""" mylog("debug", f"[New Devices] Collecting New Devices Query: {query}") current_scan_data = sql.execute(query).fetchall() diff --git a/server/scan/presence.py b/server/scan/presence.py new file mode 100644 index 000000000..1d45743a0 --- /dev/null +++ b/server/scan/presence.py @@ -0,0 +1,42 @@ +import re + +_SQL_IDENTIFIER_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$") + + +def current_scan_presence_condition(mac_column: str) -> str: + """SQL fragment: TRUE if any CurrentScan row for mac_column asserts presence. + The one and only definition of 'is this MAC present this cycle' - every + caller uses this, nobody writes their own CurrentScan presence predicate. + + mac_column MUST be a trusted, hardcoded SQL column/table.column reference + written by NetAlertX code (e.g. "devMac", "CurrentScan.scanMac") - this + function does raw string interpolation, not parameterized SQL. Never pass + plugin data, user input, or any runtime string value here. + + The inner CurrentScan is aliased as presence_scan so a qualified + mac_column like "CurrentScan.scanMac" resolves to the outer reference, + not this subquery's own row - without the alias the bare table name + shadows it, turning the comparison into a same-row tautology that's + true for any row with a non-NULL scanMac. mac_column may not reference + presence_scan itself for the same reason. + + Not usable everywhere a presence check appears: the "New Connections" + query in session_events.py and the raw Sessions insert in + create_new_devices() (device_handling.py) both need the actual + scanLastIP/scanVendor *values* off the presence-asserting row via a + MIN()/GROUP BY aggregate, not just a boolean - see + scan-pipeline-hardening.md Design §1's correction for why those two + (plus "IP Changed", an eighth non-canonical site) keep their own + hand-written aggregation instead of calling this helper. + """ + if not _SQL_IDENTIFIER_RE.match(mac_column): + raise ValueError(f"mac_column must be a plain identifier, 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 this helper's own " + f"internal subquery alias, got: {mac_column!r}" + ) + return f"""EXISTS ( + SELECT 1 FROM CurrentScan AS presence_scan + WHERE presence_scan.scanMac = {mac_column} AND presence_scan.scanPresence = 1 + )""" diff --git a/server/scan/session_events.py b/server/scan/session_events.py index a26eb3822..edca2fd37 100755 --- a/server/scan/session_events.py +++ b/server/scan/session_events.py @@ -14,6 +14,7 @@ update_presence_from_CurrentScan ) from helper import get_setting_value +from scan.presence import current_scan_presence_condition from db.db_helper import print_table_schema from utils.datetime_utils import timeNowUTC from logger import mylog, Logger @@ -190,10 +191,7 @@ def insert_events(db): AND devCanSleep = 0 AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} - AND NOT EXISTS (SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) """) + AND NOT {current_scan_presence_condition("devMac")} """) # Check device down – sleeping devices whose sleep window has expired mylog("debug", "[Events] - 1b - Devices down (sleep expired)") @@ -207,9 +205,7 @@ def insert_events(db): AND devIsSleeping = 0 AND devPresentLastScan = 0 AND {_SQL_NOT_FORCED_ONLINE} - AND NOT EXISTS (SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1) + AND NOT {current_scan_presence_condition("devMac")} AND NOT EXISTS (SELECT 1 FROM Events WHERE eveMac = devMac AND eveEventType = 'Device Down' @@ -274,10 +270,7 @@ def insert_events(db): WHERE devAlertDown = 0 AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} - AND NOT EXISTS (SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) """) + AND NOT {current_scan_presence_condition("devMac")} """) # Check IP Changed mylog("debug", "[Events] - 4 - IP Changes") diff --git a/test/db/test_schema_drift_guard.py b/test/db/test_schema_drift_guard.py new file mode 100644 index 000000000..ab30127c8 --- /dev/null +++ b/test/db/test_schema_drift_guard.py @@ -0,0 +1,172 @@ +""" +Tests for the Events/Sessions/AppEvents/Notifications schema drift guard +(server/db/schema_columns.py, server/db/db_upgrade.py's ensure_table_columns()) +- see scan-pipeline-hardening.md Design §3. + +Three things are tested: +1. No drift today between TABLE_COLUMNS and the real server/db/schema/app.sql. +2. The drift-detection logic can actually detect a real mismatch (not just a + check that always passes trivially) - per the prd-writing skill's rule + that a guard needs its own test, separate from "no drift found today". +3. AppEvents/Notifications specifically have a *second* schema-definition + surface beyond app.sql (their own inline CREATE TABLE IF NOT EXISTS in + application code) - keep that in sync too, using the real classes rather + than re-parsing their SQL as text. +4. ensure_table_columns() backfill: a table missing one column gets it added + with the right type. +""" + +import os +import sqlite3 +import sys +from unittest.mock import patch + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server")) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +from db.schema_columns import TABLE_COLUMNS # noqa: E402 +from db.db_upgrade import ensure_table_columns # noqa: E402 +from db_test_helpers import make_db, DummyDB # noqa: E402 + +_APP_SQL_PATH = os.path.join( + os.path.dirname(__file__), "..", "..", "server", "db", "schema", "app.sql" +) + + +def _columns_from_ddl(ddl_sql, table): + """Execute DDL into a fresh in-memory connection and return the + resulting {column: declared_type} map, via SQLite's own DDL parser + rather than a hand-rolled regex. Types normalized (stripped, uppercased) + so harmless casing differences aren't reported as drift.""" + conn = sqlite3.connect(":memory:") + try: + conn.executescript(ddl_sql) + return { + row[1]: row[2].strip().upper() + for row in conn.execute(f'PRAGMA table_info("{table}")').fetchall() + } + finally: + conn.close() + + +def _drift(ddl_sql, table): + """Column-level differences between TABLE_COLUMNS and ddl_sql's actual + schema for table - missing columns, extra columns, and type mismatches + on columns present in both. Empty set = no drift.""" + expected = {name: t.strip().upper() for name, t in TABLE_COLUMNS[table].items()} + actual = _columns_from_ddl(ddl_sql, table) + + drift = set() + for name in expected.keys() - actual.keys(): + drift.add(f"missing:{name}") + for name in actual.keys() - expected.keys(): + drift.add(f"extra:{name}") + for name in expected.keys() & actual.keys(): + if expected[name] != actual[name]: + drift.add(f"type:{name}({expected[name]!r} != {actual[name]!r})") + return drift + + +class TestNoDriftAgainstRealAppSql: + @pytest.mark.parametrize("table", list(TABLE_COLUMNS.keys())) + def test_table_matches_app_sql(self, table): + app_sql = open(_APP_SQL_PATH).read() + drift = _drift(app_sql, table) + assert not drift, ( + f"{table}: TABLE_COLUMNS (server/db/schema_columns.py) and " + f"app.sql disagree on {drift} - update whichever one is stale" + ) + + +class TestGuardActuallyDetectsDrift: + """Proves the comparison logic can fail, not just a check that always + reports success - without this, it's possible to ship a guard that + passes regardless of what it's given.""" + + def test_missing_columns_detected(self): + broken_sql = "CREATE TABLE Events (eveMac STRING (50), eveIp STRING (50));" + drift = _drift(broken_sql, "Events") + assert drift == { + "missing:eveDateTime", "missing:eveEventType", "missing:eveAdditionalInfo", + "missing:evePendingAlertEmail", "missing:evePairEventRowid", + } + + def test_extra_column_detected(self): + broken_sql = ( + "CREATE TABLE Sessions (sesMac STRING (50), sesUnexpectedNewColumn TEXT);" + ) + drift = _drift(broken_sql, "Sessions") + assert "extra:sesUnexpectedNewColumn" in drift + + def test_type_mismatch_detected(self): + broken_sql = "CREATE TABLE Events (eveMac INTEGER, eveIp TEXT);" + drift = _drift(broken_sql, "Events") + assert any(item.startswith("type:eveMac") for item in drift) + + +class TestInlineDDLMatchesConstant: + """AppEvents/Notifications each have a second schema-definition surface + beyond app.sql - the inline CREATE TABLE IF NOT EXISTS in their own + Python classes (found during Design §3 implementation - see + scan-pipeline-hardening.md's correction). Exercised via the real classes, + not by re-parsing their embedded SQL as text.""" + + def test_app_events_inline_ddl_matches_constant(self): + from workflows.app_events import AppEvent_obj + + conn = make_db() + db = DummyDB(conn) + AppEvent_obj(db) # drops + recreates AppEvents with the real inline DDL + + cols = {row[1] for row in conn.execute('PRAGMA table_info("AppEvents")').fetchall()} + assert cols == set(TABLE_COLUMNS["AppEvents"].keys()) + conn.close() + + def test_notifications_inline_ddl_matches_constant(self): + from models.notification_instance import NotificationInstance + + conn = make_db() + db = DummyDB(conn) + with patch("models.notification_instance.get_setting_value", return_value=""), \ + patch("models.notification_instance.Logger"): + NotificationInstance(db) + + cols = {row[1] for row in conn.execute('PRAGMA table_info("Notifications")').fetchall()} + assert cols == set(TABLE_COLUMNS["Notifications"].keys()) + conn.close() + + +class TestEnsureTableColumnsBackfill: + """ensure_table_columns() must repair a table that's missing a column - + mirroring however Devices' existing 18 ensure_column() calls are (or + aren't currently) tested, generalized to the four registered tables.""" + + @pytest.mark.parametrize("table", list(TABLE_COLUMNS.keys())) + def test_missing_column_is_backfilled(self, table): + columns = TABLE_COLUMNS[table] + first_col, first_type = next(iter(columns.items())) + remaining = {c: t for c, t in columns.items() if c != first_col} + + conn = sqlite3.connect(":memory:") + col_defs = ", ".join(f'"{c}" {t}' for c, t in remaining.items()) + conn.execute(f"CREATE TABLE {table} ({col_defs})") + cur = conn.cursor() + + ok = ensure_table_columns(cur, table) + assert ok, f"ensure_table_columns({table}) reported failure" + + info = {row[1]: row[2] for row in conn.execute(f'PRAGMA table_info("{table}")').fetchall()} + assert set(info.keys()) == set(columns.keys()), f"{table}: backfill did not restore {first_col}" + assert info[first_col].strip().upper() == first_type.strip().upper(), ( + f"{table}: {first_col} restored with type {info[first_col]!r}, expected {first_type!r}" + ) + conn.close() + + def test_missing_table_skips_without_error(self): + conn = sqlite3.connect(":memory:") + cur = conn.cursor() + ok = ensure_table_columns(cur, "Notifications") + assert ok, "a not-yet-created table must not be treated as a failure" + conn.close() diff --git a/test/scan/test_presence_helper.py b/test/scan/test_presence_helper.py new file mode 100644 index 000000000..cf906b524 --- /dev/null +++ b/test/scan/test_presence_helper.py @@ -0,0 +1,125 @@ +""" +Tests for server/scan/presence.py's current_scan_presence_condition() - +the shared predicate helper from scan-pipeline-hardening.md Design §1. + +Two things are tested: +1. The helper itself: correct SQL fragment, and rejects anything that isn't + a plain SQL identifier (the trust-boundary check - this function does raw + string interpolation, never parameterized SQL). +2. A positive, AST-based guard (not a grep for one hand-written spelling, + which is trivially defeated by an equivalent one - scanPresence <> 0, + bare scanPresence, NOT scanPresence = 0, etc.) that each migrated + consumer function's source actually calls the helper the expected number + of times. This is what stops a future change from quietly reintroducing + a hand-written predicate instead of calling the shared one. + +Not covered here on purpose: "New Connections" (session_events.py) and the +raw Sessions insert (device_handling.py's create_new_devices()) - both need +the actual scanLastIP/scanVendor *values* off the presence-asserting row via +MIN()/GROUP BY, not just a boolean, so they keep their own hand-written +aggregation - see scan-pipeline-hardening.md Design §1's correction. +""" + +import ast +import inspect +import os +import sqlite3 +import sys +import textwrap + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server")) + +from scan.presence import current_scan_presence_condition # noqa: E402 +from scan import device_handling # noqa: E402 +from scan import session_events # noqa: E402 + + +class TestHelperCorrectness: + def test_returns_expected_sql_fragment(self): + result = current_scan_presence_condition("devMac") + assert "EXISTS (" in result + assert "SELECT 1 FROM CurrentScan" in result + assert "scanMac = devMac" in result + assert "scanPresence = 1" in result + + def test_accepts_qualified_column_reference(self): + result = current_scan_presence_condition("CurrentScan.scanMac") + assert "scanMac = CurrentScan.scanMac" in result + + @pytest.mark.parametrize("bad_value", [ + "devMac; DROP TABLE Devices--", + "devMac OR 1=1", + "'; DELETE FROM Devices; --", + "devMac)", + "", + "123devMac", + ]) + def test_rejects_non_identifier_input(self, bad_value): + with pytest.raises(ValueError): + current_scan_presence_condition(bad_value) + + @pytest.mark.parametrize("bad_value", ["presence_scan", "presence_scan.scanMac"]) + def test_rejects_presence_scan_qualifier(self, bad_value): + """presence_scan is this helper's own internal subquery alias.""" + with pytest.raises(ValueError): + current_scan_presence_condition(bad_value) + + +class TestQualifiedColumnExecutesCorrectly: + """Executes the fragment, not just checks the generated SQL text - proves + a qualified mac_column ("CurrentScan.scanMac") still discriminates + per-row rather than collapsing into "does any row assert presence".""" + + def test_only_the_present_mac_matches(self): + conn = sqlite3.connect(":memory:") + conn.execute("CREATE TABLE CurrentScan (scanMac TEXT, scanPresence INTEGER)") + conn.execute("INSERT INTO CurrentScan VALUES ('aa', 1)") # present + conn.execute("INSERT INTO CurrentScan VALUES ('bb', 0)") # row exists, not present + conn.commit() + + condition = current_scan_presence_condition("CurrentScan.scanMac") + rows = conn.execute( + f"SELECT scanMac, {condition} AS is_present FROM CurrentScan" + ).fetchall() + + assert dict(rows) == {"aa": 1, "bb": 0}, ( + "each row must be checked against its own scanMac, not collapse " + "into a table-wide 'does anything assert 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 + a text grep - resilient to reformatting, doesn't care how a bypass + might be spelled, only whether the actual call is present).""" + source = textwrap.dedent(inspect.getsource(func)) + tree = ast.parse(source) + count = 0 + for node in ast.walk(tree): + if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == target_name: + count += 1 + return count + + +class TestConsumersCallTheHelper: + """Guards the five sites that were migrated to the shared predicate.""" + + def test_update_presence_from_current_scan_calls_helper_twice(self): + assert _call_count(device_handling.update_presence_from_CurrentScan) == 2, ( + "update_presence_from_CurrentScan() has two statements (present/not-present) " + "- both must call current_scan_presence_condition()" + ) + + def test_update_dev_last_connection_calls_helper_once(self): + assert _call_count(device_handling.update_devLastConnection_from_CurrentScan) == 1 + + def test_insert_events_calls_helper_at_least_three_times(self): + """insert_events() contains four queries total - Device Down (x2), + Disconnected, and New Connections. Only the first three are plain + boolean-predicate sites; New Connections keeps its own present_agg/ + MIN(scanLastIP) aggregation on purpose (see module docstring), so + this asserts >= 3, not == 4.""" + assert _call_count(session_events.insert_events) >= 3 diff --git a/test/scan/test_scan_creates_device.py b/test/scan/test_scan_creates_device.py index 6e50d92bd..23615d13d 100644 --- a/test/scan/test_scan_creates_device.py +++ b/test/scan/test_scan_creates_device.py @@ -170,3 +170,50 @@ def test_differing_ip_and_vendor_collapse_to_one_event(self): "differing scanLastIP/scanVendor across plugin rows for the same " "new MAC must not produce duplicate New Device events" ) + + +class TestBlankMacNeverCreatesDevice: + """A row with a blank/null-equivalent scanMac must never originate a + Devices row, even with scanCreatesDevice = 1 (the default) - this is the + backstop for a plugin that has rows it can't attach a real MAC to but + forgot to (or can't) set scanCreatesDevice = 0 itself. A well-behaved + plugin should still set scanCreatesDevice = 0 for such rows (see + plugin-import-behavior-controls.md) - this guard exists for the case + where it doesn't, so a blank MAC can never create a device regardless.""" + + def test_blank_scanmac_with_creates_device_one_creates_nothing(self): + conn = make_db() + insert_current_scan_row_from_dict( + conn, make_current_scan_dict("", scanCreatesDevice=1) + ) + db = DummyDB(conn) + + device_handling.create_new_devices(db) + + assert _devices(db) == set() + rows = conn.execute( + "SELECT * FROM Events WHERE eveMac = '' AND eveEventType = 'New Device'" + ).fetchall() + assert rows == [], "a blank scanMac must not produce an orphan New Device event either" + + def test_multiple_plugins_sharing_blank_scanmac_creates_nothing(self): + """The scenario this guard was actually written for: several + unrelated rows (e.g. containers with no routable MAC) all reporting + scanMac = '' collapse into one CurrentScan group - that group must + never create a device, regardless of how many rows are in it.""" + conn = make_db() + insert_current_scan_row_from_dict( + conn, make_current_scan_dict("", scanSourcePlugin="PLUGINA", scanCreatesDevice=0) + ) + insert_current_scan_row_from_dict( + conn, make_current_scan_dict("", scanSourcePlugin="PLUGINB", scanCreatesDevice=1) + ) + db = DummyDB(conn) + + device_handling.create_new_devices(db) + + assert _devices(db) == set(), ( + "even a single row asserting scanCreatesDevice = 1 for a blank MAC " + "must not create a device - most-permissive-wins does not override " + "the blank-MAC guard" + )