diff --git a/hub/internal/monitor/offsite.go b/hub/internal/monitor/offsite.go index 1bd78645..a6e88c50 100644 --- a/hub/internal/monitor/offsite.go +++ b/hub/internal/monitor/offsite.go @@ -304,8 +304,8 @@ func (oc *OffsiteChecker) pinnedTier(customerID string) bool { // window rule (a pinned tier), so the message can say „outside any clean-up window". // // R-435 (D7, `09` §3 decision 191): on a PINNED tier any fall the hub's own windows do not explain -// alarms — even one snapshot. A window that cannot say what it removed (timeout, still open) explains -// anything: no alarm, one INFO line. Non-pinned tiers (the household's NAS, which still prunes by +// alarms — even one snapshot. Each window explains at most its hub-set max_remove (a timeout or still-open +// window explains exactly that cap); when the windows cannot be read, the half-rule decides. Non-pinned tiers (the household's NAS, which still prunes by // itself) keep the half-rule below. Pinned by r435_pinned_drop_test.go. // // LIMIT: the count is the box's own (R-895) and it is a NET count — a night's new snapshots between @@ -319,15 +319,16 @@ func (oc *OffsiteChecker) snapshotDropped(customerID string, off *offsiteReport, if drop > 0 && oc.pinnedTier(customerID) { from := oc.lastCountAt[customerID].Add(-windowSlack) removed, unknown := oc.store.RemovedByWindowsBetween(customerID, from, reportAt) - if unknown { - oc.logger.Printf("[INFO] Offsite snapshot count for %s fell %d -> %d with a clean-up window that cannot say what it removed (timeout or still open) — treated as explained", customerID, prev, off.SnapshotCount) + if !unknown { + if drop > removed { + return true, prev, off.SnapshotCount, true + } + oc.logger.Printf("[DEBUG] Offsite snapshot count for %s fell %d -> %d, explained by clean-up windows (up to %d)", customerID, prev, off.SnapshotCount, removed) return false, prev, off.SnapshotCount, true } - if drop > removed { - return true, prev, off.SnapshotCount, true - } - oc.logger.Printf("[DEBUG] Offsite snapshot count for %s fell %d -> %d, explained by clean-up windows (%d removed)", customerID, prev, off.SnapshotCount, removed) - return false, prev, off.SnapshotCount, true + // Fail closed (security review 2026-10-08): the windows could not be read, so the half-rule + // decides — never „explained". + oc.logger.Printf("[INFO] Offsite snapshot count for %s fell %d -> %d and the clean-up windows could not be read — the half-rule decides", customerID, prev, off.SnapshotCount) } if drop < snapshotDropFloor { return false, prev, off.SnapshotCount, false diff --git a/hub/internal/monitor/r435_pinned_drop_test.go b/hub/internal/monitor/r435_pinned_drop_test.go index ee6955f9..efc1bd70 100644 --- a/hub/internal/monitor/r435_pinned_drop_test.go +++ b/hub/internal/monitor/r435_pinned_drop_test.go @@ -94,6 +94,7 @@ func TestR435_WindowExplainsOnlyPart(t *testing.T) { } } +// A timeout-closed window explains up to its hub-set cap (10 here): 69 → 60 is explained. func TestR435_TimeoutWindowIsUnknownNoAlarm(t *testing.T) { timeout := func(st *store.Store) { id, err := st.OpenOffsiteWindowRow("p5", time.Now().Add(30*time.Minute), 69, 10) @@ -107,6 +108,15 @@ func TestR435_TimeoutWindowIsUnknownNoAlarm(t *testing.T) { } } +// Security review 2026-10-08: a box that claims its window removed everything explains only the cap the +// hub granted (10): 69 → 40 is 29 > 10 → one alarm. RED-PROOF: drop the cap in RemovedByWindowsBetween → 0 alarms. +func TestR435_LyingWindowExplainsOnlyItsCap(t *testing.T) { + if msgs := r435Run(t, "p9", r435Setup{pinned: true, confirmed: true, + window: closedWindow(t, "p9", 69, 0), next: dropJSON(40, true, "", "ok")}); len(msgs) != 1 { + t.Fatalf("a window capped at 10 cannot explain 69 -> 40; got %d alarm(s)", len(msgs)) + } +} + // Controls: the half-rule still governs a non-pinned tier, and an installed-but-unconfirmed key. func TestR435_NotPinnedKeepsHalfRule(t *testing.T) { if msgs := r435Run(t, "n1", r435Setup{next: dropJSON(60, true, "", "ok")}); len(msgs) != 0 { diff --git a/hub/internal/store/offsite_keys.go b/hub/internal/store/offsite_keys.go index 3f384129..c3ae8895 100644 --- a/hub/internal/store/offsite_keys.go +++ b/hub/internal/store/offsite_keys.go @@ -326,18 +326,23 @@ func (s *Store) ForceOffsiteAbandonDueForTest(id int64) error { } // RemovedByWindowsBetween — R-435 (D7, `09` §3 decision 191). How many snapshots the clean-up windows -// the hub opened for this customer EXPLAIN between two reports: the sum of count_before − count_after -// over windows closed in (from, to]. On a pinned tier these windows are the only legitimate way the -// count can fall, so anything beyond the sum is unexplained. +// the hub opened for this customer EXPLAIN between two reports, over windows closed in (from, to] or +// still open. On a pinned tier these windows are the only legitimate way the count can fall, so +// anything beyond the sum is unexplained. // -// unknown = true when a window in the interval cannot say what it removed: closed by timeout (no box -// result, count_after −1 or NULL), or still open (opened at or before `to`, not closed). The caller -// then treats the fall as explained — the safe side for noise, recorded as a limit in `08`. A query -// error is unknown too. +// Each window explains AT MOST its hub-set max_remove (security review 2026-10-08): count_after is the +// box's own word, so a box that lies about it — or a window closed by timeout or still open, which has +// no count_after — can never explain more than the cap the hub itself granted. A closed window with a +// count_after explains min(count_before − count_after, max_remove); a window with no usable count_after +// explains max_remove. +// +// unknown = true only when the store cannot answer (a query error, or a window with no usable cap). The +// caller then falls back to the half-rule — never to „explained" (fail closed, the review's other +// finding). Pinned by r435_windows_between_test.go and r435_pinned_drop_test.go. func (s *Store) RemovedByWindowsBetween(customerID string, from, to time.Time) (removed int, unknown bool) { const f = "2006-01-02 15:04:05" rows, err := s.db.Query(` - SELECT count_before, count_after, closed_at IS NULL FROM offsite_windows + SELECT count_before, count_after, max_remove, closed_at IS NULL FROM offsite_windows WHERE customer_id = ? AND ((closed_at IS NULL AND opened_at <= ?) OR (closed_at > ? AND closed_at <= ?))`, customerID, to.UTC().Format(f), from.UTC().Format(f), to.UTC().Format(f)) @@ -346,17 +351,23 @@ func (s *Store) RemovedByWindowsBetween(customerID string, from, to time.Time) ( } defer rows.Close() for rows.Next() { - var before, after sql.NullInt64 + var before, after, maxRemove sql.NullInt64 var open bool - if err := rows.Scan(&before, &after, &open); err != nil { + if err := rows.Scan(&before, &after, &maxRemove, &open); err != nil { return 0, true } - if open || !before.Valid || !after.Valid || after.Int64 < 0 { + if !maxRemove.Valid || maxRemove.Int64 <= 0 { unknown = true continue } - if d := int(before.Int64 - after.Int64); d > 0 { - removed += d + explains := int(maxRemove.Int64) + if !open && before.Valid && after.Valid && after.Int64 >= 0 { + if d := int(before.Int64 - after.Int64); d < explains { + explains = d + } + } + if explains > 0 { + removed += explains } } if rows.Err() != nil { diff --git a/hub/internal/store/r435_windows_between_test.go b/hub/internal/store/r435_windows_between_test.go index 547f1da6..bf7afee2 100644 --- a/hub/internal/store/r435_windows_between_test.go +++ b/hub/internal/store/r435_windows_between_test.go @@ -5,26 +5,28 @@ import ( "time" ) -// R-435: RemovedByWindowsBetween sums only windows closed inside the interval, and says „unknown" -// for a window that cannot say what it removed. +// R-435: RemovedByWindowsBetween sums only windows closed inside the interval (or still open), and each +// window explains at most its hub-set max_remove — a box's count_after cannot widen it, and a window with +// no usable count_after explains exactly its cap. Only a window with no cap makes the answer unknown. +// RED-PROOF (security review 2026-10-08): drop the max_remove cap → „lying box" returns 69, not 34 → FAILS. func TestR435_RemovedByWindowsBetween(t *testing.T) { s := newTestStore(t) at := func(ts string) time.Time { v, _ := time.Parse("2006-01-02 15:04:05", ts); return v } - ins := func(cust, opened, closed string, before, after any) { + ins := func(cust, opened, closed string, before, after, maxRemove any) { t.Helper() var c any if closed != "" { c = closed } - if _, err := s.db.Exec(`INSERT INTO offsite_windows (customer_id, opened_at, closes_by, closed_at, count_before, count_after) VALUES (?, ?, ?, ?, ?, ?)`, - cust, opened, opened, c, before, after); err != nil { + if _, err := s.db.Exec(`INSERT INTO offsite_windows (customer_id, opened_at, closes_by, closed_at, count_before, count_after, max_remove) VALUES (?, ?, ?, ?, ?, ?, ?)`, + cust, opened, opened, c, before, after, maxRemove); err != nil { t.Fatal(err) } } - ins("a", "2026-10-01 03:00:00", "2026-10-01 03:10:00", 69, 60) // inside → 9 - ins("a", "2026-09-20 03:00:00", "2026-09-20 03:10:00", 80, 69) // before the interval - ins("a", "2026-10-03 03:00:00", "2026-10-03 03:10:00", 60, 50) // after the interval - ins("b", "2026-10-01 03:00:00", "2026-10-01 03:10:00", 50, 40) // another customer + ins("a", "2026-10-01 03:00:00", "2026-10-01 03:10:00", 69, 60, 34) // inside → 9 + ins("a", "2026-09-20 03:00:00", "2026-09-20 03:10:00", 80, 69, 40) // before the interval + ins("a", "2026-10-03 03:00:00", "2026-10-03 03:10:00", 60, 50, 30) // after the interval + ins("b", "2026-10-01 03:00:00", "2026-10-01 03:10:00", 50, 40, 25) // another customer from, to := at("2026-09-30 00:00:00"), at("2026-10-02 00:00:00") if n, unk := s.RemovedByWindowsBetween("a", from, to); n != 9 || unk { @@ -33,14 +35,24 @@ func TestR435_RemovedByWindowsBetween(t *testing.T) { if n, unk := s.RemovedByWindowsBetween("c", from, to); n != 0 || unk { t.Fatalf("no window: want 0 known, got %d unknown=%v", n, unk) } - // a timeout close (count_after −1) inside the interval → unknown - ins("d", "2026-10-01 03:00:00", "2026-10-01 03:40:00", 69, -1) - if _, unk := s.RemovedByWindowsBetween("d", from, to); !unk { - t.Fatal("a timeout-closed window must make the interval unknown") + // a box that claims it removed everything explains only the cap the hub granted + ins("l", "2026-10-01 03:00:00", "2026-10-01 03:10:00", 69, 0, 34) + if n, unk := s.RemovedByWindowsBetween("l", from, to); n != 34 || unk { + t.Fatalf("lying box: want the cap 34, got %d unknown=%v", n, unk) } - // a window still open → unknown - ins("e", "2026-10-01 03:00:00", "", 69, nil) - if _, unk := s.RemovedByWindowsBetween("e", from, to); !unk { - t.Fatal("an open window must make the interval unknown") + // a timeout close (count_after −1) inside the interval → explains its cap + ins("d", "2026-10-01 03:00:00", "2026-10-01 03:40:00", 69, -1, 10) + if n, unk := s.RemovedByWindowsBetween("d", from, to); n != 10 || unk { + t.Fatalf("timeout-closed window: want its cap 10, got %d unknown=%v", n, unk) + } + // a window still open → explains its cap + ins("e", "2026-10-01 03:00:00", "", 69, nil, 10) + if n, unk := s.RemovedByWindowsBetween("e", from, to); n != 10 || unk { + t.Fatalf("open window: want its cap 10, got %d unknown=%v", n, unk) + } + // a window with no cap → unknown (the caller falls back to the half-rule) + ins("f", "2026-10-01 03:00:00", "2026-10-01 03:10:00", 69, 60, nil) + if _, unk := s.RemovedByWindowsBetween("f", from, to); !unk { + t.Fatal("a window with no cap must make the interval unknown") } }