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