Skip to content

feat(ingestor): accept client RX coverage only from configured sources (#265) - #274

Merged
dborup merged 2 commits into
masterfrom
codex/issue-265-client-rx-sources
Oct 5, 2026
Merged

dborup merged 2 commits into
masterfrom
codex/issue-265-client-rx-sources

Conversation

@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator

Relates to #265

Plan

  1. Add an optional clientRxCoverage.sources allowlist (of mqttSources[].name) to the ingestor config, with a trimmed, case-insensitive matcher that returns "allow everything" when the list is absent, empty, or blank-only.
  2. Gate the meshcore/client/... dispatch in handleMessage on that matcher — after the observer-blacklist check, before any coverage write — and return from the client branch on a drop, so the namespace still never falls through to the observer path.
  3. Log drops per source, throttled and line-capped, mirroring the existing observerIATAWhitelist warning (iata_drop_warn.go). Throttle state lives on the Config, so there is no package-global to reset.
  4. Report, once at startup, any sources name that matches no configured source — an allowlist that can never match would otherwise drop all coverage in silence.
  5. Document the option and why it matters on a multi-broker deployment, in docs/client-rx-coverage.md and config.example.json.

Why

Trust in meshcore/client/{PUBLIC_KEY}/packets rests entirely on the broker binding the topic pubkey to the publisher that holds that key (docs/client-rx-coverage.md, "Trust"). An instance can read several brokers with different authentication — one that binds the topic to a per-device identity, plus a legacy username/password broker where many accounts may write meshcore/#. Without this option, enabling coverage trusts the weakest of them: any account on the legacy broker could inject coverage under any companion pubkey, with any GPS position. The allowlist keeps the client namespace on the sources that actually enforce the binding, while the other source keeps contributing ordinary observer traffic unchanged.

Behaviour

"mqttSources": [
  { "name": "device-auth", "broker": "mqtts://…" },
  { "name": "legacy",      "broker": "mqtts://…" }
],
"clientRxCoverage": { "enabled": true, "sources": ["device-auth"] }
  • sources set and non-empty → meshcore/client/... is handled only from a listed source. From any other source the message is dropped before any write: no client_receptions row, no client_observers row, and no observer row (the client namespace still always returns from its branch).
  • sources absent, empty, or blank-only → every source is accepted, exactly as before. Upstream-compatible default.
  • Matching is on the source name, trimmed and case-insensitive. A source with no name can never be listed, so it is rejected whenever an allowlist is set.
  • Drops are logged per source: first drop immediately, re-logged at most every 10 minutes, and never more than 10 lines per source for the lifetime of the process (the last line says so).
  • An unknown name is logged once at boot, alongside a line naming the effective restriction.
  • The observer blacklist still runs first, so it holds whatever the source.

Files

  • cmd/ingestor/config.go — ClientRxCoverageConfig.Sources, ClientRxCoverageSources(), ClientRxSourceAllowed(), per-Config throttle field.
  • cmd/ingestor/client_rx_sources.go (new) — throttled/bounded drop warning + checkClientRxSources startup validation.
  • cmd/ingestor/main.go — the dispatch guard and the startup call.
  • cmd/ingestor/client_rx_sources_test.go (new) — tests, all ingest assertions through the real handleMessage path with a named source.
  • docs/client-rx-coverage.md, config.example.json — documentation.

cmd/server is untouched (it ignores the new field; its read endpoints stay gated by enabled alone). No new map[string]interface{}. Fork guards unchanged: 9 in deploy.yml, 1 in release-fast-path.yml.

dborup added 2 commits October 5, 2026 19:55
Trust in meshcore/client/{PUBLIC_KEY}/packets rests entirely on the broker
binding the topic pubkey to the publisher holding that key. An instance that
reads several brokers can mix one that enforces that binding with a legacy
username/password broker where many accounts may write meshcore/#: enabling
coverage there trusts the weakest source, and any account on it could inject
coverage under any companion pubkey with any GPS position.

clientRxCoverage.sources is an optional allowlist of mqttSources[].name. When
set and non-empty, the client namespace is handled only for messages that
arrived on a listed source; a message from any other source is dropped before
any write (no client_receptions, no client_observers, and no observer row — the
namespace still always returns), logged per source with a throttle and a line
cap. Absent or empty keeps every source accepted, so the default is unchanged.
A name matching no configured source is reported once at startup, since an
allowlist that can never match would otherwise drop all coverage silently.

The blacklist check still runs first, so it holds whatever the source.

Relates to #265
…is off

The boot line reported "coverage restricted to N MQTT source(s)" even with
clientRxCoverage.enabled false, where the allowlist is inert and no coverage is
ingested from any source. Name that state instead of implying the listed
sources are contributing.

Relates to #265
@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-Macmini PR#274 #265 — head e75c190

Status: Implemented and verified locally; draft PR open, CI green (one unrelated E2E flake re-run once, detailed below). Evidence tags: [T] test, [A] analysis/code reading, [K] command output.

Acceptance criteria

# Requirement How it is met Test Mutant
1 clientRxCoverage.sources is an optional list of mqttSources[].name; set and non-empty ⇒ meshcore/client/... handled only from listed sources ClientRxCoverageConfig.Sources + Config.ClientRxSourceAllowed (cmd/ingestor/config.go), enforced in the client branch of handleMessage (cmd/ingestor/main.go) before the coverage call [A] TestClientRxSourceAllowedWithList, TestClientRxCoverageListedSourceIngests [T] Helper hard-wired to return true ⇒ TestClientRxSourceAllowedWithList, TestClientRxCoverageUnlistedSourceDropsEverything, TestClientRxCoverageUnnamedSourceDropped fail [K]
1 From a non-listed source the message is dropped: no client_receptions, no client_observers, no observer row Guard returns from the client branch before handleClientPacket; the namespace still always returns, so nothing reaches the observer path [A] TestClientRxCoverageUnlistedSourceDropsEverything (all-table row-count delta = ∅ via assertOnlyDeltas, plus explicit counts on both coverage tables), TestClientRxCoverageUnnamedSourceDropped, TestClientRxCoverageUnlistedSourceOtherSubtopicDropped [T] Source check deleted from handleMessage ⇒ TestClientRxCoverageUnlistedSourceDropsEverything + TestClientRxCoverageUnnamedSourceDropped fail [K]
1 Logged with throttling and a line cap warnClientRxSourceDrop + per-Config clientRxSourceDropThrottle (cmd/ingestor/client_rx_sources.go): first drop logs, re-log at most every 10 min per source, hard cap of 10 lines per source for the process lifetime (last line says so). Mirrors the observerIATAWhitelist warning in iata_drop_warn.go; state is per-Config, so no package global and no cross-test leakage [A] TestClientRxSourceDropLogThrottled (500 drops in-window ⇒ 1 line; per-source window; reopens after the interval), TestClientRxSourceDropLogCapped (cap exactly 10, last line marked) [T] Interval check removed ⇒ TestClientRxSourceDropLogThrottled fails; cap check removed ⇒ TestClientRxSourceDropLogCapped fails [K]
2 Without sources, or with an empty list, behaviour is unchanged ClientRxCoverageSources() strips blank entries and returns nil for absent/empty; ClientRxSourceAllowed then allows every source, unnamed included [A] TestClientRxSourceAllowedWithoutList (nil config / absent / blank-only), TestClientRxCoverageNoAllowlistUnchanged (coverage ingested from an arbitrary source name), plus the pre-existing TestClientRxCoverageGateOn/Off, TestClientTopicGateOn/Off* suites unchanged and green [T] Blank entries not stripped ⇒ TestClientRxSourceAllowedWithoutList/blank-only_sources + TestCheckClientRxSourcesSilentWithoutAllowlist fail [K]
3 A name matching no configured source is logged once at startup checkClientRxSources(cfg, sources) called once in main right after cfg.ResolvedSources(); logs the effective restriction plus one WARNING naming the unknown entries, and returns them [A] TestCheckClientRxSourcesUnknownName (exactly one warning line, names the unknown entry, configured names not reported), TestCheckClientRxSourcesSilentWithoutAllowlist (no allowlist ⇒ no output), TestCheckClientRxSourcesDisabledSaysSo (an allowlist set while the feature is off is reported as inert, not as an active restriction) [T] — (startup path, asserted directly on the captured log)
4 Blacklist check and "client namespace always returns" hold whatever the source The blacklist check is untouched and still runs first in the branch; both the allowlist drop and the unchanged paths return from the branch [A] TestClientRxCoverageBlacklistBeatsAllowlist (blacklisted companion on a LISTED source ⇒ nothing written), TestClientRxCoverageUnlistedSourceOtherSubtopicDropped (/status sub-topic from an unlisted source writes nothing — no observer, no metrics), plus the pre-existing TestClientRxCoverageBlacklistedDropped and the client_topic_dispatch_test.go suite [T] — (covered by the two deletion mutants above, which also make these paths write)
5 Tests go through the real handleMessage with a source tag dispatchFrom builds an MQTTSource{Name: …} and calls handleMessage directly — no shortcut into handleClientPacket [A][T] all ingest tests above see above
6 Docs: docs/client-rx-coverage.md + config docs, incl. why it matters with several brokers New section "Restricting which MQTT sources may contribute" (what it does, JSON example, the mixed-broker rationale, matching rules, ingest-only scope), a new step 3 in "Enabling coverage", a bullet in "Trust", and _comment_clientRxCoverage_sources in config.example.json [A] config.example.json still parses as JSON [K] —
7 cmd/server untouched; no new map[string]interface{}; fork guards unchanged The commit touches 6 files, none under cmd/server or .github (git show --name-only); the diff adds no map[string]interface{}; grep -c 'Kpa-clawbot/CoreScope' = 9 in deploy.yml, 1 in release-fast-path.yml [K] cmd/server suite green (shared config) [K] —

Behaviour summary

"mqttSources": [
  { "name": "device-auth", "broker": "mqtts://…" },
  { "name": "legacy",      "broker": "mqtts://…" }
],
"clientRxCoverage": { "enabled": true, "sources": ["device-auth"] }

Matching is on the source name, trimmed and case-insensitive; a source with no name can never be listed and is therefore rejected whenever an allowlist is set. The option gates the ingest write path only — the read endpoints remain gated by enabled alone, so only the ingestor needs the restart.

Local verification [K]

Command Result
cd cmd/ingestor && go test ./... ok, 114.5 s (re-run at the final head)
cd cmd/ingestor && go vet ./... clean
gofmt -l on the four touched Go files clean (the repo's pre-existing unformatted files are untouched)
cd cmd/server && go vet ./... && go test ./... ok, 41.1 s
sh test-all.sh 219 passed, 0 failed (219 files)

Mutants run one at a time against the committed tests, then reverted [K]:

Mutant Red tests
Source check removed from handleMessage TestClientRxCoverageUnlistedSourceDropsEverything, TestClientRxCoverageUnnamedSourceDropped
ClientRxSourceAllowed always allows + TestClientRxSourceAllowedWithList
Blank entries not stripped from sources TestClientRxSourceAllowedWithoutList/blank-only_sources, TestCheckClientRxSourcesSilentWithoutAllowlist
Throttle interval ignored TestClientRxSourceDropLogThrottled
Line cap removed TestClientRxSourceDropLogCapped

CI per job

Run 37352751184, head e75c1901 [K]:

Job Result
✅ Go Build & Test pass (19m32s) — includes cmd/server + cmd/ingestor build/test with coverage, the ingestor race check, the shared packages, sh test-all.sh, the preflight XSS gate and the frontend lint
🎭 Playwright E2E Tests pass (19m41s) on the second attempt — see below
🏗️ Build & Publish Docker Image pass (image builds; the GHCR push steps are fork-guarded and did not run)
📦 Release Artifacts skipping (fork guard / tag-only)
🚀 Deploy Staging skipping (fork guard / push-only)
📝 Publish Badges & Summary skipping (fork guard / push-only)

One flaky E2E failure, re-run once. The first E2E attempt failed on a single step in
test-issue-180-packets-url-modal-e2e.js — "desktop (1400): Clear Filters on #/packets/?… —
reload shows 7 packets, Clear showed 8" (11 passed, 1 failed). The step compares the row count right
after Clear Filters (which restores the default 15-minute window) with the count after a reload a
second later, so a fixture packet sitting on the window boundary ages out between the two reads; the
test's own comment acknowledges that the fixture "may have aged out of the default 15 min window". It
is time-dependent and unrelated to this change, whose diff touches only cmd/ingestor/,
docs/client-rx-coverage.md and config.example.json — no frontend, no API, no server code. The
failed job was re-run once and passed. Note this is a different flake from the known #256 Hash
Stats sort one; worth its own issue if it recurs.

Remaining / notes

  • Two commits: the feature, then a follow-up making the boot line honest when sources is set while clientRxCoverage.enabled is false (the allowlist is inert there).

  • Branch base is ae126374; master has since advanced to the flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256 Hash Stats sort fix. Not rebased (out of scope for this PR), and the two do not overlap.

  • The drop-warning interval (10 min) and line cap (10) are constants, not config. The observerIATAWhitelist warning next to it exposes iataWarnIntervalSec; this one did not need a knob for the acceptance criteria, and adding one would widen the config surface. Easy to promote later if an operator wants it.

  • ClientRxSourceAllowed scans the allowlist linearly instead of building a cached set (as ObserverBlacklist does). The list is bounded by the number of configured MQTT sources, so the set would cost more than it saves; noted in the doc comment.

  • The server ignores the new field (unknown JSON fields are dropped), so no cmd/server change was needed. If the read side ever needs to know about the restriction, that is a separate change.

@dborup-agent

Copy link
Copy Markdown
Collaborator

Review — CS-pve-agent1 PR#274 — head e75c190

Dom: APPROVE with nits

Independent, read-only review. Evidence tags: [T] test I ran, [A] analysis / code reading, [K] command output. Everything was run on the head archive and on the merged tree git merge-tree --write-tree origin/master e75c1901… (tree f5e0d0cd, master c6b356de, merges without conflict). Relative to master, the merged tree differs only in the 6 files this PR touches [K].

Findings

# Severity Finding Evidence
1 Nit (test gap) No test pins the JSON key clientRxCoverage.sources through LoadConfig. Every PR test builds ClientRxCoverageConfig{Sources: …} directly, so a renamed/typoed struct tag (json:"sourcez") keeps the whole PR suite green while the documented config key silently does nothing (allowlist ignored ⇒ every source accepted, the fail-open direction). Suggest one test that writes a config.json with two mqttSources + sources and asserts ClientRxCoverageSources() after LoadConfig. Mutant M3 below survived all PR tests, killed only by my scratch test [T]
2 Nit (hardening) Matching is case-insensitive, but nothing enforces unique mqttSources[].name (mqttSourceTags only de-dupes unnamed sources). Two sources named auth and Auth (or identically) are both admitted by one allowlist entry. Since this option is a trust boundary, checkClientRxSources could warn when an allowlisted name matches more than one configured source. Operator-error scenario only, not a blocker. cmd/ingestor/config.go ClientRxSourceAllowed, mqtt_client_id.go mqttSourceTags [A]
3 Nit (docs, AGENTS rule 8) The 10-minute re-log interval and 10-line cap are hardcoded (acknowledged in the author's report) but not listed under "Configurable values (future customizer)" in docs/client-rx-coverage.md. [A]
4 Info CI attempt 1 E2E failure confirmed as test-issue-180-packets-url-modal-e2e.js "reload shows 7 packets, Clear showed 8" (11 passed, 1 failed). This is a time-window flake, not #256 or #267, and the PR touches nothing it exercises. Locally the same file passes 12/0 on the merged tree. attempt-1 job log [K], local run [T]

Answers to the review points

  1. Source filter. With sources set, the check sits in the client branch of handleMessage, after the blacklist check and before handleClientPacket. A drop is followed by return, so nothing reaches client_receptions/client_observers and the namespace never falls through to the observer path [A]. My scratch test loads a real config.json with two sources (Device-Auth, legacy; allowlist [" device-auth "]) via LoadConfig + ResolvedSources(), then drives the real handleMessage with each resolved MQTTSource on one store:

    • 50 coverage messages from legacy ⇒ zero row delta on every data table, exactly one drop log line;
    • the same packet from Device-Auth (case and whitespace differ) ⇒ exactly client_receptions +1, client_observers +1, no observer row;
    • ordinary observer traffic from legacy still writes transmissions +1, observers +1 (unchanged);
    • meshcore/client/<pk>/status from legacy ⇒ zero delta. [T]

    Throttle (first line, then ≤1 per 10 min per source) and cap (10 lines, last one says so) are covered by TestClientRxSourceDropLogThrottled/Capped. The throttle map is keyed by configured source name, so it is bounded [A][T].

  2. No sources / empty list. ClientRxCoverageSources() returns nil for absent, empty or blank-only lists, and ClientRxSourceAllowed then returns true. With both a config without the key and one with "sources": [], coverage from both sources is ingested and the startup check prints nothing [T]. Pre-existing TestClientRxCoverageGateOn/Off and TestClientTopic* stay green [T]. An unknown name produces exactly one WARNING at startup, naming the entry (TestCheckClientRxSourcesUnknownName, plus my JSON-loaded variant with a typo) [T]. checkClientRxSources is called once in main() right after ResolvedSources() [A].

  3. Blacklist and the always-return rule. The blacklist check is unchanged and still runs first. A blacklisted companion on a listed source writes nothing (TestClientRxCoverageBlacklistBeatsAllowlist) [T]. Removing the outer client-branch return (M2) turns 6 test functions red, including the PR's …UnlistedSourceOtherSubtopicDropped. Removing the inner drop return (M1) turns 3 red [T]. An unlisted-source message cannot reach the observer path [A][T].

  4. Config. The server's ClientRxCoverageConfig has only Enabled, so sources is an ignored unknown field. Server LoadConfig silently falls back to defaults on any json.Unmarshal error, so I checked this explicitly: a scratch server test loads a config.json carrying sources (non-empty and []) and ClientRxCoverageEnabled() stays true [T]. A real server started with that config reports clientRxCoverage: true in /api/config/client [K]. cmd/server and .github are untouched (git diff --name-only empty) [K].

  5. Docs. The new section, enabling step 3, Trust bullet and _comment_clientRxCoverage_sources match the code: name matching is trimmed and case-insensitive, unnamed sources are rejected, drops are throttled and capped, the startup warning is real, it applies only to ingest, and the read side is gated by enabled alone. "Restart the ingestor" is correct: SIGHUP reloads only hashChannels/hashRegions [A]. Anchors resolve. config.example.json parses [K]. Examples use only generic names (device-auth, legacy, mqtts://…). The diff adds no hosts, IPs or deployment details [A].

Always-checks:

  • No behaviour change beyond the gate, which only runs when enabled is set and the topic is …/packets [A].
  • No new map[string]interface{} (0 added lines) [K].
  • No hardcoded colours: the hex-pattern hits are #265 comments only [K].
  • bash scripts/check-xss-sinks.sh --diff origin/master reports no public/ changes, exit 0 [K].
  • Fork guards: 9 in deploy.yml and 1 in release-fast-path.yml, on both head and the merged tree [K].
  • No closing keywords in the commits or the PR body ("Relates to ingestor: accept client RX coverage only from configured MQTT sources #265") [K].
  • Both commits are authored and committed by dborup <kontakt@meshview.dk> [K].
  • go vet is clean for cmd/ingestor and cmd/server [K].

Acceptance criteria → test, red before / green after

On base the PR tests do not compile (no Sources field). Behaviourally, mutant M4 (check removed) equals the base dispatch and M3 equals base config parsing; both are shown red below.

Acceptance criterion Test(s) Red under
Listed ingests, unlisted writes nothing, no observer row …ListedSourceIngests, …UnlistedSourceDropsEverything, …UnnamedSourceDropped, reviewer two-source test M1, M4
Absent/empty unchanged …AllowedWithoutList, …NoAllowlistUnchanged, existing gate suites (blank-strip mutant per author)
Blacklist + always-return …BlacklistBeatsAllowlist, …UnlistedSourceOtherSubtopicDropped, TestClientTopic* M2, M6
Docs n/a n/a
Real handleMessage path with source tag + mutant dispatchFrom → handleMessage M4
cmd/server untouched, no new maps n/a n/a

Tests and mutants

On the merged tree:

Command Result
cd cmd/ingestor && go test -count=1 -timeout 30m ./... ok, 648 s [K]
cd cmd/server && go test -count=1 -timeout 30m ./... ok, 575 s [K]
sh test-all.sh 219 passed, 0 failed [K]
node test-frontend-helpers.js 707 passed, 0 failed [K]

Note: my first parallel run of both suites hit Go's default 10-minute -timeout on this slow host, in TestWriterStarvationVisibleInPerf and TestFallbackRelays_…_202. Neither had a --- FAIL. Both suites pass when run sequentially with -timeout 30m. This is a host issue, not a PR issue.

My own mutants, one at a time, run against the PR tests plus TestClientTopic*, *Blacklist* and my scratch tests, then reverted:

Mutant Result
M1: drop-path return removed killed: …UnlistedSourceDropsEverything, …UnnamedSourceDropped, reviewer test
M2: outer client-branch return removed killed: TestClientTopicGateOff/OnWrites*, …UnsupportedSubtopicsWriteNothing (10 subtests), …FloodHop/FloodTrace, …UnlistedSourceOtherSubtopicDropped, reviewer test
M3: JSON tag sources → sourcez survives the PR suite, killed only by the reviewer tests (finding 1)
M4: ClientRxSourceAllowed check bypassed killed: …UnlistedSourceDropsEverything, …UnnamedSourceDropped, reviewer test
M5: case-sensitive match killed: …AllowedWithList, reviewer test
M6: client blacklist check removed killed: …BlacklistBeatsAllowlist, …BlacklistedDropped, TestClientTopicBlacklistedCompanionWritesNothing
M7: unknown-name startup warning silenced killed: TestCheckClientRxSourcesUnknownName, reviewer test
M8: incoming source name not trimmed killed: …AllowedWithList

E2E against a local Go server, merged tree, e2e-fixture.db prepared as in CI:

  • Preparation: freshen, inline seed SQL from deploy.yml, corescope-migrate, then seeds 2073, 199 and 245 (245 because current CI also applies it). The server was stopped by port.

Default config (coverage off, as in CI):

  • test-issue-180-packets-url-modal-e2e.js: 12/0 [T]
  • test-node-reach-e2e.js: OK [T]
  • test-node-reach-coverage-e2e.js and test-rx-coverage-mobile-nav-e2e.js: SKIP (coverage disabled), as in CI [T]

Config with enabled: true and sources:

  • test-rx-coverage-mobile-nav-e2e.js: 3/0 [T]
  • test-node-reach-e2e.js: OK [T]
  • test-node-reach-coverage-e2e.js: fails with query failed, because the fixture has no client_* tables. Those are created by the ingestor, not by corescope-migrate, and CI never runs this test with coverage enabled. This is a fixture limitation, independent of this PR: the merged tree's cmd/server and public/ match master byte for byte [K][A].

CI (run 37352751184, attempt 2, head e75c1901): Go Build & Test pass, Playwright E2E pass, Docker build pass; Release, Deploy and Badges skipped (fork guard). The attempt-1 E2E failure is described in finding 4 [K].

Not verified

  • No live MQTT run. No broker was available locally, so the ingestor binary was not run against two real brokers. The two-source evidence comes from the real LoadConfig → ResolvedSources → handleMessage path in-process, not from MQTT delivery.
  • Startup call not executed. I saw the call to checkClientRxSources in main() but did not run it: main is not unit-testable, so deleting the call would be a surviving mutant by construction [A].
  • test-e2e-playwright.js incomplete locally. It stops fail-fast at step 7 ("Version info lives on Perf dashboard", #navStats wait timeout) on two runs. A standalone probe fills #navStats in about 250 ms. cmd/server, public/ and the test file are identical to master and CI passes the job, so I treat this as environmental, but the remaining steps of that file were not run locally.
  • No browser validation of UI changes. There are none in this PR.

No pushes, merges, ready-state changes or issue creation; all work was in scratch copies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants