fix(ingestor): defense-in-depth empty-scope guard in UpdateNodeDefaultScope (#1534) (#1575)

## 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 <bot@meshcore-analyzer>

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)

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)
+	}
+}

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=<advert bytes>) 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)
+	}
+}