From 675c576fead83ea504cc7314abdba550f26028d1 Mon Sep 17 00:00:00 2001 From: efiten Date: Thu, 10 Sep 2026 22:30:24 +0200 Subject: [PATCH] fix(scope-audit): bound the verifier's payload, and fix two tests that proved less than they claimed (#1993) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three leftovers from reviewing the scope-audit series (#1986, #1987, #1990). None is urgent; all three are the kind of thing that gets harder to explain the longer it sits. ## `scopeHMACInputs` accepted a payload `DecodePacket` rejects Its comment says it walks the same offsets as the decoder, and it does, minus the `maxPacketPayload` bound the decoder enforces. Unreachable in practice: such a packet never reaches the database with an empty `scope_name` in the first place, so the verifier never sees one. Worth closing anyway, because the comment claims the two agree. A verifier that accepts what the decoder refuses is a small divergence today and an hour of confusion on the day it matters. ## The corroboration test seeded the same packet twice The threshold of two rests on `code1` being two bytes: one match is 1/65536 by chance, two on the same region is (1/65536)². That argument needs two **independent** observations. The test seeded `realTransportFloodPacket` twice. Identical payloads derive identical codes, so it was one observation counted twice, and it would have passed just as happily against an implementation that counted rows rather than deriving anything. It now seeds the real captured packet plus a second one built for a different payload, both deriving to `#fm-112` on their own. **The feature was never wrong here.** `transmissions.hash` is unique and `ComputeContentHash` is path-independent, so two rows always mean two distinct payloads in production. Only the test failed to demonstrate the property it is named for. ## Naming at ingest and verifying at read time had no test together They were built separately, in #1990 and #1989, and the interaction between them is not exotic: with derived region keys enabled, a packet that used to be stored unnameable now arrives with a name. The audit must then report that region as observed by the ordinary route: - present in `agg.scopes` - absent from `notObserved` - **not** claimed by `regionEvidence`, which exists to explain regions that could only be established by verification Getting that wrong is quiet. The chip stays green while the reason underneath it is wrong, and a reader asking "how do we know this" gets the wrong story. ## Verification `cd cmd/server && go test ./...` passes (254s), `go vet` and `gofmt -l` clean. No production behaviour changes beyond the payload bound, which rejects input that cannot occur. --- cmd/server/scope_audit_test.go | 58 +++++++++++++++++++++++++++++++++- cmd/server/scope_verify.go | 9 +++++- 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/cmd/server/scope_audit_test.go b/cmd/server/scope_audit_test.go index 749cfec7..3ebd811b 100644 --- a/cmd/server/scope_audit_test.go +++ b/cmd/server/scope_audit_test.go @@ -3,6 +3,7 @@ package main import ( "context" "database/sql" + "encoding/hex" "encoding/json" "fmt" "net/http" @@ -1180,13 +1181,32 @@ func TestHandleScopeAuditSurfacesUnmatchedPackets(t *testing.T) { // forwarded are stored unmatched. Verification derives the key from the // repeater's own declaration, finds two corroborating packets, and the region // must leave notObserved with its evidence count reported. +// transportFloodPacketFor builds a TRANSPORT_FLOOD packet whose code1 is the +// code `region` derives over this payload, so the verifier will match it. The +// shape mirrors realTransportFloodPacket: header 0x14 (route 0, payload type +// 5), the two transport codes, path byte 0x41 (hash size 2, one hop), the hop, +// then the payload. +// +// It exists because corroboration has to come from DIFFERENT payloads. Two +// copies of one packet derive the same code by construction, so they are one +// observation counted twice, not the two independent matches the threshold +// argument rests on. +func transportFloodPacketFor(region string, payload []byte) string { + code1 := regionCode(region, 5, payload) + return "14" + code1 + "0000" + "41" + "E3D3" + strings.ToUpper(hex.EncodeToString(payload)) +} + func TestHandleScopeAuditVerifiesDeclaredRegion(t *testing.T) { srv, router := setupScopeAuditServer(t) pk := testFullPubkeyA insertDeclared(t, srv, pk, time.Now().UTC().Format(time.RFC3339), "fm-112,behss", 0) recent := time.Now().UTC().Add(-time.Minute).Format(time.RFC3339) + // Two DIFFERENT payloads, each deriving to #fm-112 on its own. The first is + // the real packet captured from a live instance; the second is built for + // this test. Seeding the same packet twice would prove only that the + // verifier counts rows. seedUnmatchedRawAt(t, srv.store, pk[:4], realTransportFloodPacket, RouteTransportFlood, recent) - seedUnmatchedRawAt(t, srv.store, pk[:4], realTransportFloodPacket, RouteTransportFlood, recent) + seedUnmatchedRawAt(t, srv.store, pk[:4], transportFloodPacketFor("fm-112", []byte{0x51, 0x52, 0x53, 0x54, 0x55}), RouteTransportFlood, recent) got := getScopeAudit(t, router, "") if len(got.Repeaters) != 1 { @@ -1292,3 +1312,39 @@ func seedUnmatchedRawAt(t *testing.T, s *PacketStore, forwarder, rawHex string, // TestHandleNodeScopesDifferentWindowIsSeparateCacheEntry confirms the cache // key includes window: a request for a different window must recompute // rather than reuse another window's cached entry. + +// TestHandleScopeAuditNamedRegionNeedsNoVerification pins the interaction +// between naming a packet at ingest and verifying it at read time, which are +// built separately and had no test together. +// +// Deriving region keys from what nodes declare means a packet that used to be +// stored unnameable now arrives with a name. The audit must then report that +// region as observed by the ordinary route: present in agg.scopes, absent from +// notObserved, and NOT claimed by regionEvidence, which exists to explain +// regions that could only be established by verification. +// +// Getting this wrong is not loud. A region would still be green, so the page +// looks right while the reason underneath it is wrong, and a reader chasing +// "how do we know this" is told the wrong story. +func TestHandleScopeAuditNamedRegionNeedsNoVerification(t *testing.T) { + srv, router := setupScopeAuditServer(t) + pk := testFullPubkeyA + insertDeclared(t, srv, pk, time.Now().UTC().Format(time.RFC3339), "fm-112", 0) + recent := time.Now().UTC().Add(-time.Minute).Format(time.RFC3339) + // scopeMatched is the state the ingestor writes once it holds a key for + // the region, whether that key was configured by hand or derived. + seedTransmissionRouteAt(t, srv.store, pk[:4], scopeMatched("#fm-112"), RouteTransportFlood, recent) + + row := getScopeAudit(t, router, "").Repeaters[0] + for _, rgn := range row.NotObserved { + if rgn == "fm-112" { + t.Errorf("notObserved = %v, must not contain fm-112 — it was observed under its own name", row.NotObserved) + } + } + if n, ok := row.RegionEvidence["fm-112"]; ok { + t.Errorf("regionEvidence[fm-112] = %d, want absent — verification explains regions that could not be named, and this one was", n) + } + if row.ObservedUnmatchedPackets != 0 { + t.Errorf("observedUnmatchedPackets = %d, want 0 — a named packet is not unnameable traffic", row.ObservedUnmatchedPackets) + } +} diff --git a/cmd/server/scope_verify.go b/cmd/server/scope_verify.go index 4f460ef2..3988df44 100644 --- a/cmd/server/scope_verify.go +++ b/cmd/server/scope_verify.go @@ -99,7 +99,14 @@ func scopeHMACInputs(rawHex string) (payloadType byte, payload []byte, code1 str return 0, nil, "", false } rest := buf[offset:] - if len(rest) == 0 { + if len(rest) == 0 || len(rest) > maxPacketPayload { + // The upper bound mirrors DecodePacket, which rejects a payload past + // the firmware's MAX_PACKET_PAYLOAD. Unreachable in practice, because + // such a packet never reaches the database with an empty scope_name in + // the first place, but the comment above claims this walks the same + // offsets as the decoder and that should be true rather than nearly + // true. A verifier that accepts what the decoder rejects is a small + // divergence today and a confusing one to debug later. return 0, nil, "", false } return byte(header.PayloadType), rest, code1, true