From 7480cc6b24abb164ba03b7ae2816a6253b4384d2 Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 01:40:01 +0000 Subject: [PATCH 1/3] test(ingestor): pin clientRxCoverage.sources through LoadConfig; reproduce #278 gaps Loads the documented key through the real LoadConfig, so a typo in the struct tag (json:"sourcez") no longer leaves the suite green while the allowlist silently fails open. Also reproduces the missing startup warning for an allowlisted name that matches several mqttSources[].name, and the missing doc entries for the hardcoded drop-log interval and line cap. Relates to #278 Co-Authored-By: Claude Opus 5.5 --- cmd/ingestor/client_rx_sources_278_test.go | 121 +++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 cmd/ingestor/client_rx_sources_278_test.go diff --git a/cmd/ingestor/client_rx_sources_278_test.go b/cmd/ingestor/client_rx_sources_278_test.go new file mode 100644 index 000000000..e39c594a8 --- /dev/null +++ b/cmd/ingestor/client_rx_sources_278_test.go @@ -0,0 +1,121 @@ +package main + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +// Follow-ups to the #265 client-RX source allowlist (#278). + +// TestLoadConfigClientRxCoverageSources pins the documented JSON key +// clientRxCoverage.sources through the real LoadConfig. Every other allowlist +// test builds ClientRxCoverageConfig directly, so a typo in the struct tag +// would leave them green while the key silently did nothing — and an ignored +// allowlist fails open (every source accepted). +func TestLoadConfigClientRxCoverageSources(t *testing.T) { + t.Setenv("MQTT_BROKER", "") + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, []byte(`{ + "mqttSources": [ + {"name": "a", "broker": "tcp://a.invalid:1883", "topics": ["meshcore/#"]}, + {"name": "b", "broker": "tcp://b.invalid:1883", "topics": ["meshcore/#"]} + ], + "clientRxCoverage": { "enabled": true, "sources": ["a"] } + }`), 0o644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadConfig(cfgPath) + if err != nil { + t.Fatal(err) + } + if cfg.ClientRxCoverage == nil || !cfg.ClientRxCoverageEnabled() { + t.Fatalf("clientRxCoverage.enabled not loaded: %+v", cfg.ClientRxCoverage) + } + if got := cfg.ClientRxCoverage.Sources; len(got) != 1 || got[0] != "a" { + t.Fatalf("clientRxCoverage.sources = %q, want [a]", got) + } + if !cfg.ClientRxSourceAllowed("a") { + t.Error("listed source a must be allowed") + } + if cfg.ClientRxSourceAllowed("b") { + t.Error("unlisted source b must be rejected once sources is loaded") + } +} + +// TestCheckClientRxSourcesDuplicateName: matching is case-insensitive and +// mqttSources[].name is not required to be unique, so one allowlisted name can +// admit several brokers. That widens a trust boundary without the operator +// noticing, so startup warns once per such name and lists the matching sources. +func TestCheckClientRxSourcesDuplicateName(t *testing.T) { + sources := []MQTTSource{{Name: "auth"}, {Name: " Auth "}, {Name: "legacy"}, {Name: "LEGACY"}} + + cfg := coverageCfgWithSources("AUTH") + var unknown []string + out := captureLog(t, func() { unknown = checkClientRxSources(cfg, sources) }) + + if len(unknown) != 0 { + t.Fatalf("a duplicated name is not unknown: unknown = %v", unknown) + } + var warn []string + for _, l := range strings.Split(out, "\n") { + if strings.Contains(l, "WARNING") { + warn = append(warn, l) + } + } + if len(warn) != 1 { + t.Fatalf("expected exactly one warning line, got %d:\n%s", len(warn), out) + } + for _, want := range []string{"AUTH", `"auth"`, `" Auth "`, "2"} { + if !strings.Contains(warn[0], want) { + t.Errorf("warning must mention %s:\n%s", want, warn[0]) + } + } + if strings.Contains(strings.ToLower(warn[0]), "legacy") { + t.Errorf("legacy is duplicated but not allowlisted, it must not be reported:\n%s", warn[0]) + } +} + +// TestCheckClientRxSourcesUniqueNamesNoDuplicateWarning: one match per +// allowlisted name ⇒ no warning at all. +func TestCheckClientRxSourcesUniqueNamesNoDuplicateWarning(t *testing.T) { + sources := []MQTTSource{{Name: "auth"}, {Name: "legacy"}} + out := captureLog(t, func() { checkClientRxSources(coverageCfgWithSources("Auth", "legacy"), sources) }) + if strings.Contains(out, "WARNING") { + t.Fatalf("unique names must not warn:\n%s", out) + } +} + +// TestClientRxCoverageDocListsDropLogLimits: AGENTS.md rule 8 — the hardcoded +// drop-warning interval and line cap are listed under "Configurable values +// (future customizer)", with their current values taken from the constants so +// the docs cannot drift from the code. +func TestClientRxCoverageDocListsDropLogLimits(t *testing.T) { + data, err := os.ReadFile(filepath.Join("..", "..", "docs", "client-rx-coverage.md")) + if err != nil { + t.Fatal(err) + } + const heading = "## Configurable values (future customizer)" + doc := string(data) + i := strings.Index(doc, heading) + if i < 0 { + t.Fatalf("docs/client-rx-coverage.md has no %q section", heading) + } + section := doc[i+len(heading):] + if j := strings.Index(section, "\n## "); j >= 0 { + section = section[:j] + } + section = strings.Join(strings.Fields(section), " ") // tolerate line wrapping + + for _, want := range []string{ + fmt.Sprintf("`clientRxSourceWarnInterval`, %d minutes", int(clientRxSourceWarnInterval.Minutes())), + fmt.Sprintf("`clientRxSourceWarnMax`, %d lines", clientRxSourceWarnMax), + } { + if !strings.Contains(section, want) { + t.Errorf("configurable-values section must list %q; got:\n%s", want, section) + } + } +} From 0257da3970a14acfa24937d2ac21ccd6ed5f597a Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 01:40:01 +0000 Subject: [PATCH 2/3] fix(ingestor): warn when a clientRxCoverage.sources name matches several sources (#278) mqttSources[].name is not required to be unique and matching is case-insensitive, so one allowlisted name can admit several brokers. checkClientRxSources now logs one startup WARNING listing every such name and the sources it matches. Warning only; no behaviour change. Co-Authored-By: Claude Opus 5.5 --- cmd/ingestor/client_rx_sources.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/cmd/ingestor/client_rx_sources.go b/cmd/ingestor/client_rx_sources.go index 95dc6933d..dfbd51fb6 100644 --- a/cmd/ingestor/client_rx_sources.go +++ b/cmd/ingestor/client_rx_sources.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "log" "strings" "sync" @@ -91,24 +92,29 @@ func (c *Config) warnClientRxSourceDrop(tag, name string, now time.Time) { // checkClientRxSources reports, once at startup, every clientRxCoverage.sources // entry that matches no configured mqttSources[].name — an allowlist of names // that can never match would silently drop all coverage. It also logs the -// effective restriction so the gate is visible in the boot log. Returns the +// effective restriction so the gate is visible in the boot log, and warns when +// one entry matches more than one configured source (#278). Returns the // unknown names (nil when there is no allowlist or every name matches). func checkClientRxSources(cfg *Config, sources []MQTTSource) []string { allow := cfg.ClientRxCoverageSources() if len(allow) == 0 { return nil } - var unknown []string + var unknown, ambiguous []string for _, want := range allow { - found := false + var matched []string for _, src := range sources { if strings.EqualFold(strings.TrimSpace(src.Name), want) { - found = true - break + matched = append(matched, fmt.Sprintf("%q", src.Name)) } } - if !found { + switch { + case len(matched) == 0: unknown = append(unknown, want) + case len(matched) > 1: + // mqttSources[].name is not required to be unique and matching is + // case-insensitive, so one entry can admit several brokers (#278). + ambiguous = append(ambiguous, fmt.Sprintf("%s matches %d sources (%s)", want, len(matched), strings.Join(matched, ", "))) } } state := "" @@ -122,5 +128,9 @@ func checkClientRxSources(cfg *Config, sources []MQTTSource) []string { log.Printf("[client-rx] WARNING: %d clientRxCoverage.sources name(s) match no configured mqttSources[].name: %s — coverage will never be accepted for those names; check the spelling", len(unknown), strings.Join(unknown, ", ")) } + if len(ambiguous) > 0 { + log.Printf("[client-rx] WARNING: clientRxCoverage.sources name(s) match more than one configured mqttSources[].name (case-insensitive): %s — coverage is accepted from every one of them; give each source a unique name", + strings.Join(ambiguous, "; ")) + } return unknown } From 3db11114cc6f0c8fec3e8780227a9210ff68039f Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 01:40:01 +0000 Subject: [PATCH 3/3] docs(client-rx): list the drop-log interval and line cap as configurable values (#278) AGENTS.md rule 8: clientRxSourceWarnInterval (10 minutes) and clientRxSourceWarnMax (10 lines) are hardcoded; list them under "Configurable values (future customizer)" and document the duplicate-name startup warning. Co-Authored-By: Claude Opus 5.5 --- docs/client-rx-coverage.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/client-rx-coverage.md b/docs/client-rx-coverage.md index edb2da83b..328ade780 100644 --- a/docs/client-rx-coverage.md +++ b/docs/client-rx-coverage.md @@ -65,6 +65,9 @@ Notes: can never be listed, so it is rejected whenever an allowlist is set. - A name that matches no configured source is logged once at startup — the allowlist would otherwise silently drop all coverage from that (mistyped) name. +- Source names are not required to be unique, so a name that matches more than one configured source + (for example `auth` and `Auth`) admits all of them; that is also logged once at startup. Give each + source a unique name. - Drops are logged per source, throttled and capped at a fixed number of lines, so a busy unlisted broker cannot flood the log. - The option gates only the client namespace. The observer blacklist and every other ingest rule stay @@ -251,3 +254,10 @@ on-device signing) — not required for the MVP, tracked as a follow-up. Hardcoded initially, tracked for the customizer per AGENTS.md rule 8: hex resolution per zoom (`zoomToHexRes`), colour SNR thresholds (`coverageColorVar`), and any `rx_at` max-age validation. + +The source-allowlist drop warning ([Restricting which MQTT sources may +contribute](#restricting-which-mqtt-sources-may-contribute)) is also hardcoded, in +`cmd/ingestor/client_rx_sources.go`: the re-log interval per source (`clientRxSourceWarnInterval`, +10 minutes) and the per-source line cap for the lifetime of the process (`clientRxSourceWarnMax`, +10 lines). These are ingestor settings, so they would become config keys rather than customizer +controls.