fix(scope-audit): bound the verifier's payload, and fix two tests that proved less than they claimed (#1993)

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.
This commit is contained in:
efiten
2026-09-10 22:30:24 +02:00
committed by GitHub
parent b5c7612166
commit 675c576fea
2 changed files with 65 additions and 2 deletions
+57 -1
View File
@@ -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)
}
}
+8 -1
View File
@@ -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