From cd19285f7f8c5f8894048e9a224baf83c106c7dd Mon Sep 17 00:00:00 2001 From: Kpa-clawbot Date: Thu, 4 Jun 2026 10:35:46 -0700 Subject: [PATCH] fix(ingestor): defense-in-depth empty-scope guard in UpdateNodeDefaultScope (#1534) (#1575) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Follow-up to PR #1569 (merged). Adds defense-in-depth at the DB layer for the #1534 default_scope-overwrite class of bug. PR #1569 fixed #1534 by guarding the call site in `handleMessage` with `if shouldUpdateDefaultScope(pktData)`. Adversarial review of #1569 flagged this as one-layer defense: a future refactor that drops the call-site `if` and calls `store.UpdateNodeDefaultScope(pubkey, pktData.ScopeName)` unconditionally would silently re-introduce the bug — overwriting a previously-correct `default_scope` (e.g. `#belgium`) with the empty string. This PR adds the belt-and-braces guard recommended by that review: - `Store.UpdateNodeDefaultScope(pk, "")` is now a silent no-op (early `return nil`) - New DB-layer regression test that fails on `master` and proves the DB function used to write `""` straight through - Two new call-site anchor tests that drive a transport-scoped ADVERT end-to-end through `handleMessage` (matched + unmatched region key) so the existing call-site guard from #1569 can't be deleted without a test going red Net production change: 8 lines in `cmd/ingestor/db.go`. No behavior change for any non-empty scope. ## Why this is a follow-up, not a re-fix Issue #1534 is already closed by #1569 and `master` no longer regresses for users (the call-site guard is in place). This PR is purely belt-and-braces — it adds the second layer of defense the adversarial reviewer asked for and the test coverage that anchors both layers. ## Files changed | File | Change | |------|--------| | `cmd/ingestor/db.go` | +8 — empty-scope early return in `UpdateNodeDefaultScope` | | `cmd/ingestor/db_test.go` | +43 — `TestUpdateNodeDefaultScope_EmptyScopeIsNoop` | | `cmd/ingestor/main_test.go` | +97 — `TestHandleMessageAdvert_EmptyScopeSkipsDefaultScopeUpdate` + `TestHandleMessageAdvert_MatchedScopeUpdatesDefaultScope` | ## Red → green commits - **red** `c062af59` — `test(ingestor): red — DB-layer empty-scope guard regression test for #1534` - Adds three tests; `TestUpdateNodeDefaultScope_EmptyScopeIsNoop` fails on assertion (`default_scope` overwritten with `""`) - Two call-site tests pass already (call-site guard merged in #1569) — they anchor that behavior against future refactors - **green** `7ab12d53` — `fix(ingestor): defense-in-depth empty-scope guard in UpdateNodeDefaultScope (#1534)` - Adds the early-return; all three tests green ## Operator remediation (from issue #1534) Operators whose production DB still has rows where `default_scope` was overwritten with the empty string before #1569 deployed can clean up with: ```sql -- Inspect affected rows first SELECT public_key, name, default_scope FROM nodes WHERE default_scope = ''; SELECT public_key, name, default_scope FROM inactive_nodes WHERE default_scope = ''; -- Convert empty-string default_scope back to NULL so the next valid -- matched-scope advert can re-populate it cleanly. UPDATE nodes SET default_scope = NULL WHERE default_scope = ''; UPDATE inactive_nodes SET default_scope = NULL WHERE default_scope = ''; ``` After #1569 + this PR are deployed, no new rows can be created with `default_scope = ''` from this code path. ## Test plan ```bash cd cmd/ingestor && go test ./... -count=1 # ok github.com/corescope/ingestor ~98s ``` ## Preflight Clean — PII, branch scope, red commit, CSS-var defined, CSS self-fallback, LIKE-on-JSON, sync migration, async-migration gate, XSS sinks all pass. No warnings. --------- Co-authored-by: Kpa-clawbot --- cmd/ingestor/db.go | 8 ++++ cmd/ingestor/db_test.go | 43 +++++++++++++++++ cmd/ingestor/main_test.go | 97 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+) diff --git a/cmd/ingestor/db.go b/cmd/ingestor/db.go index 5dc53b65..e43a9c61 100644 --- a/cmd/ingestor/db.go +++ b/cmd/ingestor/db.go @@ -1463,7 +1463,15 @@ func scopeNameForDB(data *PacketData) *string { // node. Skips the UPDATE when the stored value already matches to avoid // redundant writes on the hot MQTT ingest path. Updates both nodes and // inactive_nodes to stay consistent. +// +// Defense-in-depth (#1534): an empty scope is treated as a no-op. The call +// site at handleMessage is the primary guard (shouldUpdateDefaultScope), +// but this layer refuses the invalid write so a future caller cannot +// reintroduce the bug by passing "" directly. func (s *Store) UpdateNodeDefaultScope(pubkey, scope string) error { + if scope == "" { + return nil + } // Short-circuit: skip if already stored. var cur sql.NullString row := s.db.QueryRow(`SELECT default_scope FROM nodes WHERE public_key = ?`, pubkey) diff --git a/cmd/ingestor/db_test.go b/cmd/ingestor/db_test.go index 3d964d14..64b66831 100644 --- a/cmd/ingestor/db_test.go +++ b/cmd/ingestor/db_test.go @@ -2917,3 +2917,46 @@ func TestSchemaMultibyteSupColumns(t *testing.T) { } store2.Close() } + +// TestUpdateNodeDefaultScope_EmptyScopeIsNoop is the DB-layer defense-in-depth +// regression test for #1534. Even if the call-site guard at main.go:720 is +// later removed or refactored, the DB function MUST refuse to overwrite a +// previously-correct default_scope with the empty string. This is the +// belt-and-braces guard recommended by adversarial review (MAJOR-2) and +// dijkstra review (MINOR-2). +func TestUpdateNodeDefaultScope_EmptyScopeIsNoop(t *testing.T) { + dir := t.TempDir() + dbPath := filepath.Join(dir, "test.db") + store, err := OpenStore(dbPath) + if err != nil { + t.Fatalf("OpenStore: %v", err) + } + defer store.Close() + + if _, err := store.db.Exec(`INSERT INTO nodes (public_key, name, default_scope) VALUES ('pk1', 'Node1', '#belgium')`); err != nil { + t.Fatalf("insert node: %v", err) + } + if _, err := store.db.Exec(`INSERT INTO inactive_nodes (public_key, name, default_scope) VALUES ('pk1', 'Node1', '#belgium')`); err != nil { + t.Fatalf("insert inactive node: %v", err) + } + + // Empty-scope call must be a silent no-op (return nil), NOT overwrite. + if err := store.UpdateNodeDefaultScope("pk1", ""); err != nil { + t.Fatalf("UpdateNodeDefaultScope(\"\") returned error: %v (want nil)", err) + } + + var got string + if err := store.db.QueryRow(`SELECT default_scope FROM nodes WHERE public_key = 'pk1'`).Scan(&got); err != nil { + t.Fatalf("read nodes.default_scope: %v", err) + } + if got != "#belgium" { + t.Errorf("nodes.default_scope after empty-scope call = %q, want #belgium (DB-layer guard missing — #1534)", got) + } + var gotInactive string + if err := store.db.QueryRow(`SELECT default_scope FROM inactive_nodes WHERE public_key = 'pk1'`).Scan(&gotInactive); err != nil { + t.Fatalf("read inactive_nodes.default_scope: %v", err) + } + if gotInactive != "#belgium" { + t.Errorf("inactive_nodes.default_scope after empty-scope call = %q, want #belgium (DB-layer guard missing — #1534)", gotInactive) + } +} diff --git a/cmd/ingestor/main_test.go b/cmd/ingestor/main_test.go index 73c9a974..ffb6138f 100644 --- a/cmd/ingestor/main_test.go +++ b/cmd/ingestor/main_test.go @@ -2,8 +2,10 @@ package main import ( "bytes" + "database/sql" "encoding/hex" "encoding/json" + "fmt" "math" "os" "path/filepath" @@ -1088,3 +1090,98 @@ func TestBuildPacketDataScopeMatchingNoMatch(t *testing.T) { t.Errorf("shouldUpdateDefaultScope = true for empty ScopeName; want false (would overwrite default_scope with \"\")") } } + +// TestHandleMessageAdvert_EmptyScopeSkipsDefaultScopeUpdate is the call-site +// regression test for #1534. It drives a transport-scoped ADVERT whose +// region key does NOT match any configured region (so ScopeName=="") through +// handleMessage end-to-end and asserts that a pre-existing default_scope on +// the node is NOT overwritten with the empty string. This anchors the +// call-site guard at main.go:720 — a future refactor that drops the +// `if shouldUpdateDefaultScope(...)` wrapper and calls +// `store.UpdateNodeDefaultScope(pubkey, pktData.ScopeName)` unconditionally +// would re-introduce the #1534 bug and fail this test. +func TestHandleMessageAdvert_EmptyScopeSkipsDefaultScopeUpdate(t *testing.T) { + store := newTestStore(t) + source := MQTTSource{Name: "test"} + + // A transport-scoped ADVERT: header byte 0x10 = route_type 0 + // (TRANSPORT_FLOOD) + payload_type 4 (ADVERT). Code1=AABB (non-zero, so + // IsTransportScoped becomes true), Code2=0000, path_byte=00, then a + // 100-byte ADVERT payload (32-byte pubkey starting 46D62D… + 4-byte ts + // + 64-byte signature) reused from TestHandleMessageAdvertWithTelemetry. + const rawHex = "10AABB00000046D62DE27D4C5194D7821FC5A34A45565DCC2537B300B9AB6275255CEFB65D840CE5C169C94C9AED39E8BCB6CB6EB0335497A198B33A1A610CD3B03D8DCFC160900E5244280323EE0B44CACAB8F02B5B38B91CFA18BD067B0B5E63E94CFC85F758A8530B9240933402E0E6B8F84D5252322D52" + const pubkey = "46d62de27d4c5194d7821fc5a34a45565dcc2537b300b9ab6275255cefb65d84" + + // Pre-seed the node with a non-empty default_scope so we can detect an + // erroneous overwrite with "". + if _, err := store.db.Exec(`INSERT INTO nodes (public_key, name, default_scope) VALUES (?, 'Node1', '#belgium')`, pubkey); err != nil { + t.Fatalf("seed node: %v", err) + } + + // Empty regionKeys → matchScope() returns "" for any Code1 → ScopeName "". + msg := &mockMessage{ + topic: "meshcore/SJC/obs1/packets", + payload: []byte(`{"raw":"` + rawHex + `"}`), + } + handleMessage(store, "test", source, msg, nil, map[string][]byte{}, &Config{}) + + var got sql.NullString + if err := store.db.QueryRow(`SELECT default_scope FROM nodes WHERE public_key = ?`, pubkey).Scan(&got); err != nil { + t.Fatalf("read default_scope: %v", err) + } + if !got.Valid || got.String != "#belgium" { + t.Errorf("default_scope after empty-scope advert = %q (valid=%v), want #belgium — call-site guard at main.go:720 is missing or broken (#1534)", got.String, got.Valid) + } +} + +// TestHandleMessageAdvert_MatchedScopeUpdatesDefaultScope is the positive +// counterpart: a transport-scoped ADVERT whose Code1 matches a configured +// region key MUST cause default_scope to be updated to the matched region +// name. Together with the empty-scope test above this proves the call-site +// branch routes correctly for both ScopeName states. +func TestHandleMessageAdvert_MatchedScopeUpdatesDefaultScope(t *testing.T) { + store := newTestStore(t) + source := MQTTSource{Name: "test"} + + // Same ADVERT bytes; this time we compute the matching region key for + // the (payloadType=4, payload=) tuple so matchScope() will + // return "#de". + const advertBytes = "46D62DE27D4C5194D7821FC5A34A45565DCC2537B300B9AB6275255CEFB65D840CE5C169C94C9AED39E8BCB6CB6EB0335497A198B33A1A610CD3B03D8DCFC160900E5244280323EE0B44CACAB8F02B5B38B91CFA18BD067B0B5E63E94CFC85F758A8530B9240933402E0E6B8F84D5252322D52" + const pubkey = "46d62de27d4c5194d7821fc5a34a45565dcc2537b300b9ab6275255cefb65d84" + + advertRaw, _ := hex.DecodeString(advertBytes) + // Derive the region key whose HMAC produces Code1 we can plant in the + // header. Choose key = first 16 bytes of HMAC-SHA256(zeros, advertBytes) + // is non-deterministic to find; instead pick an arbitrary key and + // compute Code1 from it, then build the packet around that Code1. + regionKey, _ := hex.DecodeString("0123456789abcdef0123456789abcdef") + mac := hmacSHA256(regionKey, append([]byte{4}, advertRaw...)) + // Per firmware (#1534 helper logic): Code1 is the first 2 bytes of the + // HMAC, sentinel-shifted so 0x0000 → 0x0001 and 0xFFFF → 0xFFFE. + code := uint16(mac[0]) | (uint16(mac[1]) << 8) + if code == 0x0000 { + code = 0x0001 + } else if code == 0xFFFF { + code = 0xFFFE + } + code1 := fmt.Sprintf("%02X%02X", byte(code&0xFF), byte(code>>8)) + rawHex := "10" + code1 + "000000" + advertBytes + + if _, err := store.db.Exec(`INSERT INTO nodes (public_key, name, default_scope) VALUES (?, 'Node1', '#old')`, pubkey); err != nil { + t.Fatalf("seed node: %v", err) + } + + msg := &mockMessage{ + topic: "meshcore/SJC/obs1/packets", + payload: []byte(`{"raw":"` + rawHex + `"}`), + } + handleMessage(store, "test", source, msg, nil, map[string][]byte{"#de": regionKey}, &Config{}) + + var got sql.NullString + if err := store.db.QueryRow(`SELECT default_scope FROM nodes WHERE public_key = ?`, pubkey).Scan(&got); err != nil { + t.Fatalf("read default_scope: %v", err) + } + if !got.Valid || got.String != "#de" { + t.Errorf("default_scope after matched-scope advert = %q (valid=%v), want #de", got.String, got.Valid) + } +}