fix(ingestor): quote config values in client-RX source warnings (#302)

[dborup]

Relates to #302

The four nits from the review of #290 (N1–N4), all in cmd/ingestor. Log text only — checkClientRxSources is still warning-only and its return value and the gate it reports are unchanged.

Plan

Tests first, red before the fix, then the code, then at least one mutant per item.

#	Item	Change	Test
N1	Log forgery	Every config-derived value in the three startup lines goes through %q (new quoteNames helper); the ambiguous clause quotes the allowlist entry too	TestCheckClientRxSourcesQuotesConfigValues
N2	Two surviving mutants	— (test only)	TestCheckClientRxSourcesAmbiguousMessageShape
N3	Contradictory wording	Both warnings switch to the conditional while clientRxCoverage.enabled is false	TestCheckClientRxSourcesDisabledWordingAgrees
N4	Duplicates repeated	dedupeNamesFold collapses case-only duplicates in the allowlist before reporting, keeping the first spelling	TestCheckClientRxSourcesDeduplicatesAllowlist

Details

N1 — log forgery. All three lines interpolate operator-supplied config values. On origin/master an allowlist entry containing a newline splits the line it appears in and forges a further [client-rx] … record. From a real ingestor boot on origin/master, with sources: ["auth", "AUTH", "evil\n[client-rx] WARNING: forged boot line"]:

… [ingestor] [client-rx] coverage restricted to 3 MQTT source(s): auth, AUTH, evil
[client-rx] WARNING: forged boot line (clientRxCoverage.enabled is false, so no coverage is ingested at all)
… [ingestor] [client-rx] WARNING: 1 clientRxCoverage.sources name(s) match no configured mqttSources[].name: evil
[client-rx] WARNING: forged boot line — coverage will never be accepted for those names; check the spelling
… [ingestor] [client-rx] WARNING: … (case-insensitive): auth matches 2 sources ("auth", " AUTH "); AUTH matches 2 sources ("auth", " AUTH ") — coverage is accepted from every one of them; give each source a unique name

The forged records carry no timestamp prefix, which is the tell — but nothing downstream has to notice that. The same boot on this branch:

… [ingestor] [client-rx] coverage restricted to 2 MQTT source(s): "auth", "evil\n[client-rx] WARNING: forged boot line" (clientRxCoverage.enabled is false, so no coverage is ingested at all)
… [ingestor] [client-rx] WARNING: 1 clientRxCoverage.sources name(s) match no configured mqttSources[].name: "evil\n[client-rx] WARNING: forged boot line" — coverage would never be accepted for those names; check the spelling
… [ingestor] [client-rx] WARNING: … (case-insensitive): "auth" matches 2 sources ("auth", " AUTH ") — coverage would be accepted from every one of them; give each source a unique name

One log.Printf is one record again, and N3 and N4 are visible in the same three lines. The input is operator-controlled, so this is hardening, not an exploitable path.

N2 — the two surviving mutants. TestCheckClientRxSourcesAmbiguousMessageShape uses an allowlist whose first entry matches three sources and whose second matches two, then asserts the clause text verbatim:

"auth" matches 3 sources ("auth", "Auth", " AUTH "); "legacy" matches 2 sources ("legacy", "LEGACY")

A count of 3 kills len(matched) → 2, and the verbatim "; " kills the separator mutant. Both are run below.

N3 — consistent disabled wording. The state line already says clientRxCoverage.enabled is false, so no coverage is ingested at all, while the warnings two lines later claimed coverage is accepted / will never be accepted. Both now read would be accepted / would never be accepted when the feature is off, and are unchanged when it is on. The test pins both directions, so a swapped condition is red either way.

N4 — duplicates. Matching is case-insensitive, so ["auth","AUTH"] is one gate written twice (a plausible copy/paste) and reporting each spelling repeated the whole clause. dedupeNamesFold collapses the allowlist case-insensitively at the top of checkClientRxSources, keeping the first spelling, so the clause and the unknown-name entry appear once and the state line counts distinct names. The returned unknown slice is deduplicated as a consequence; nothing reads it (main.go:79 ignores it) and the allowlist itself is untouched — ClientRxCoverageSources() and ClientRxSourceAllowed() are unchanged, so which sources are admitted does not change.

Tests

Red before — the new test file on origin/master, all four tests fail: the N1 subtests see 4 log records where 2 are expected and the raw value in the log, N2 gets the unquoted clause, N3 sees the present tense under enabled: false, N4 sees the clause three times and unknown = ["typo-broker" "TYPO-BROKER"].

Mutants — six, each applied alone to the fixed tree and reverted from a snapshot, run against TestCheckClientRxSources*|TestClientRxSource*|TestClientRxCoverage*|TestLoadConfigClientRx*:

Mutant	Result
M1: quoteNames renders %s instead of %q	killed — N1 (both subtests), N4 (state-line assertion)
M2: ambiguous clause back to %s for the allowlist entry	killed — N1 (ambiguous subtest), N2
M3: strings.Join(ambiguous, "; ") → " " (the #290 survivor)	killed — N2
M4: len(matched) → literal 2 (the #290 survivor)	killed — N2
M5: mood never switches (present tense while disabled)	killed — N3
M6: dedupeNamesFold becomes a no-op	killed — N4 (both subtests)

Both mutants named in the issue now die, and every item has at least one dedicated killer.

Scope and invariants

• Diff is 2 files: cmd/ingestor/client_rx_sources.go and the new cmd/ingestor/client_rx_sources_302_test.go. Nothing under cmd/server, cmd/migrate, public/ or .github — verified byte-identical to origin/master.
• No new map[string]interface{} on any added line, tests included.
• No hardcoded colours; the only hex-shaped matches in the diff are the #302 issue references.
• scripts/check-xss-sinks.sh --diff origin/master is clean (no public/ changes).
• Fork guards unchanged: 9 in deploy.yml, 1 in release-fast-path.yml.
• Docs need no change: docs/client-rx-coverage.md describes the duplicate-name warning but does not quote its text, and the "Configurable values" section is untouched, so TestClientRxCoverageDocListsDropLogLimits still holds.
• Perf: startup only, O(allowlist × sources) over a handful of entries, plus one small map for the dedup.

🤖 Generated with Claude Code

[dborup]

Rapport — CS-Minimax PR#326 #302 — head f9e3c49

Status: All four follow-ups (N1–N4) implemented in cmd/ingestor only, each with a test that is red before and green after and at least one mutant that kills it; the two mutants named in the issue are both dead; CI is green on the first attempt (all three real jobs success, three skipped by their guards).

Evidence tags: [T] test I ran, [A] code reading, [K] command output.

Requirements

#	Requirement	Change	Test (red before → green after)	Mutant(s) killed
N1	Quote config values in the client-RX source warnings	New quoteNames helper; all three startup lines render config-derived values with %q, including the allowlist entry in the ambiguous clause	TestCheckClientRxSourcesQuotesConfigValues — before: 4 log records where 2 are expected, raw value in the log, 0 quoted occurrences; after: 2 records, raw value absent [T][K]	M1 quoteNames → %s; M2 ambiguous clause → %s [K]
N2	Stricter test must kill both #290 survivors	Test only	TestCheckClientRxSourcesAmbiguousMessageShape — pins the clause verbatim: "auth" matches 3 sources ("auth", "Auth", " AUTH "); "legacy" matches 2 sources ("legacy", "LEGACY"). Red before (unquoted entry) [T]	M3 strings.Join(ambiguous, "; ") → " " killed; M4 len(matched) → literal 2 killed [K]
N3	Consistent wording when enabled: false	Both warnings switch to the conditional (would be accepted / would never be accepted) while coverage is off; unchanged when on	TestCheckClientRxSourcesDisabledWordingAgrees (subtests disabled and enabled) — red before on all four assertions [T][K]	M5 mood never switches [K]
N4	Deduplicate case-insensitively before reporting	dedupeNamesFold collapses case-only duplicates at the top of checkClientRxSources, keeping the first spelling	TestCheckClientRxSourcesDeduplicatesAllowlist — before: clause 3× and unknown = ["typo-broker" "TYPO-BROKER"]; after: once each, state line counts distinct names [T][K]	M6 dedupeNamesFold becomes a no-op [K]

Six mutants total, each applied alone to the fixed tree and reverted from a scratchpad snapshot (not via git checkout, which would also have discarded the fix), run against TestCheckClientRxSources*|TestClientRxSource*|TestClientRxCoverage*|TestLoadConfigClientRx*. All six killed; no survivors [K].

Real boot log, before and after [T]

Built the ingestor from origin/master and from this head, and ran each against the same config file — sources: ["auth", "AUTH", "evil\n[client-rx] WARNING: forged boot line"], enabled: false, sources named auth and " AUTH ". This exercises the main.go:79 call site that the #290 review noted has no test of its own.

origin/master — five records from three log.Printf calls; the two without a timestamp prefix are forged by the config value, the clause is repeated, and the present tense contradicts the state line:

… [ingestor] [client-rx] coverage restricted to 3 MQTT source(s): auth, AUTH, evil
[client-rx] WARNING: forged boot line (clientRxCoverage.enabled is false, so no coverage is ingested at all)
… [ingestor] [client-rx] WARNING: 1 clientRxCoverage.sources name(s) match no configured mqttSources[].name: evil
[client-rx] WARNING: forged boot line — coverage will never be accepted for those names; check the spelling
… [ingestor] [client-rx] WARNING: … (case-insensitive): auth matches 2 sources ("auth", " AUTH "); AUTH matches 2 sources ("auth", " AUTH ") — coverage is accepted from every one of them; give each source a unique name

This head — three records, all four fixes visible at once:

… [ingestor] [client-rx] coverage restricted to 2 MQTT source(s): "auth", "evil\n[client-rx] WARNING: forged boot line" (clientRxCoverage.enabled is false, so no coverage is ingested at all)
… [ingestor] [client-rx] WARNING: 1 clientRxCoverage.sources name(s) match no configured mqttSources[].name: "evil\n[client-rx] WARNING: forged boot line" — coverage would never be accepted for those names; check the spelling
… [ingestor] [client-rx] WARNING: … (case-insensitive): "auth" matches 2 sources ("auth", " AUTH ") — coverage would be accepted from every one of them; give each source a unique name

Suites I ran [K]

Command	Result
cmd/ingestor: go vet ./...	clean
cmd/ingestor: go test -count=1 -timeout 40m ./...	ok, 108.6 s
cmd/server: go vet ./... && go test -count=1 -timeout 40m ./...	ok, 46.9 s
sh test-all.sh	225 passed, 0 failed (225 files)
node test-frontend-helpers.js	709 passed, 0 failed
gofmt -l on the two touched files	clean

E2E [T]

Local Go server on a scratch copy of e2e-fixture.db, prepared as in CI: tools/freshen-fixture.sh, the inline #1486 seed SQL extracted from deploy.yml, corescope-migrate, then the 2073, 199 and 245 seed files. Port taken from 13700 after checking it was free; the server stopped by the pid resolved from the listening port, and the port verified free afterwards [K].

• Coverage off (the CI default): test-node-reach-e2e.js OK; test-reach-rank-e2e.js 14/0; test-issue-1630-reach-mobile-e2e.js 7/0; test-rx-coverage-mobile-nav-e2e.js and test-node-reach-coverage-e2e.js SKIP (coverage disabled), as in CI.
• Coverage on, with a config carrying the duplicated allowlist ["a","A"]: /api/config/client reports clientRxCoverage: true, so the extra sources key and its duplicates do not disturb the server's narrower struct; test-rx-coverage-mobile-nav-e2e.js 3/0; test-node-reach-e2e.js OK. test-node-reach-coverage-e2e.js fails with query failed — the same fixture limitation reported in feat(ingestor): accept client RX coverage only from configured sources (#265) #274 and test(ingestor): pin clientRxCoverage.sources through LoadConfig (#278) #290: the client_* tables are created by the ingestor, not by corescope-migrate, and CI never runs this test with coverage on. Unrelated here — cmd/server and cmd/migrate are byte-identical to origin/master on this branch [K][A].

Always-checks [K]

• Diff is 2 files: cmd/ingestor/client_rx_sources.go (+45/−7) and the new cmd/ingestor/client_rx_sources_302_test.go. git diff --stat origin/master -- cmd/server cmd/migrate public .github is empty, so cmd/server stays read-only.
• No new map[string]interface{} on any added line, tests included (0 matches).
• No hardcoded colours: the only hex-shaped matches in the diff are the #302 issue references.
• bash scripts/check-xss-sinks.sh --diff origin/master: exit 0, no public/ changes to scan.
• Fork guards unchanged: 9 in deploy.yml, 1 in release-fast-path.yml.
• No closing keywords in the two commit messages, the title, or the body; the body opens with "Relates to Follow-ups to #290: quote config values in client-RX source warnings, pin message details, consistent disabled-coverage wording #302".
• Both commits authored and committed by dborup <kontakt@meshview.dk>. No rebase, amend or force-push.
• Behaviour: log text only. checkClientRxSources is warning-only; ClientRxCoverageSources() and ClientRxSourceAllowed() are untouched, so which sources are admitted does not change. The returned unknown slice is deduplicated as a side effect of N4; main.go:79 discards it.

CI per job

Run 37472108114, attempt 1, head f9e3c49c — no re-runs [K]:

Job	Conclusion
Go Build & Test	success (21m17s)
Playwright E2E Tests	success (25m44s), first attempt; the known flake #271 did not appear
Build & Publish Docker Image	success (50s)
Release Artifacts	skipped (tag-only / fork guard)
Deploy Staging	skipped (push-only / fork guard)
Publish Badges & Summary	skipped (push-only / fork guard)

Only one run exists for this branch, so nothing was re-run to get green.

Remaining / notes

• main() has no unit test. The call site is now covered by the manual before/after boot run above, but not by an automated test [A].
• No live MQTT broker. The warnings are verified on captured log output and on a real ingestor boot against an unreachable broker, not against real brokers.
• The state line now counts distinct names. With ["auth","AUTH"] it reads restricted to 1 MQTT source(s): "auth" rather than 2. That is the effective gate, and it is pinned by the N4 test, but it is a visible change to a line the issue did not name explicitly.
• Docs untouched. docs/client-rx-coverage.md describes the duplicate-name warning without quoting its text, so nothing there drifted; TestClientRxCoverageDocListsDropLogLimits still passes.
• Not verified: no staging or production access, no server API key used, no browser/visual validation (no UI change), and only the reach and coverage E2Es were run — against plain public/ rather than the coverage-instrumented copy CI builds.
• No merge, no ready-for-review, no issue state change.

[dborup-agent]

Review — CS-pve-agent2 PR#326 — head f9e3c49

Dom: APPROVE with nits

Independent, read-only review. Evidence tags: [T] test or probe I ran myself, [A] code reading, [K] command output I checked.

The fix does what #302 asks: all four items are fixed, each new test is red on origin/master and green on the fix, and both #290 survivors are now killed. I ran 11 mutants of my own and the tests killed all of them. The nits below are not blockers. The main one is a pre-existing %s interpolation of a config value in the client-RX drop warning. The issue does not name that warning, but it is the same class of bug as N1.

Findings

#	Severity	Where	Finding	Evidence
F1	Nit / follow-up (pre-existing)	cmd/ingestor/client_rx_sources.go:84, :88	The two client-RX drop warnings print tag with %s. tag is mqttSourceTag(source), which is mqttSources[].name, a config value. A name with an embedded newline still splits the record. Only the second occurrence of the name (source %q) is quoted. The doc comment at :73 says "The source name is quoted (%q) so an odd config value cannot forge log lines", which is only half true.	[T] probe: Name: "evil\nMQTT [auth] forged" → 2 records, the second starting MQTT [auth] forged] [client-rx] dropping client coverage …
F2	Nit (cosmetic, edge case)	dedupeNamesFold	The dedup key is strings.ToLower, but matching uses strings.EqualFold. They disagree on a few Unicode folds. For example, "s" and "ſ" (U+017F) are equal under EqualFold but not under ToLower. So ["s","ſ"] still repeats the clause and the state line counts 2. This has no security or gating impact.	[T] probe: EqualFold=true, state line restricted to 2 MQTT source(s): "s", "ſ", and the ambiguous clause appears twice
F3	Nit (wording)	PR body ("its return value … unchanged") and fix commit message ("the function's return value … unchanged")	The returned unknown slice is now deduplicated. The PR body says so further down, and the N4 test pins [typo-broker]. The only caller (main.go:79) discards it, so behaviour is unaffected. Only the summary sentence is inaccurate.	[A]
F4	Note (already disclosed by author)	state line	With ["auth","AUTH"] the state line now says restricted to 1 MQTT source(s) instead of 2. I agree this is the effective gate and the correct reading. It is pinned by the N4 test.	[A][T]

Nothing blocking.

Answers to the review points

1. N1 (log forgery): fixed. [A] All config-derived values in the three startup lines now render with %q: the state-line allowlist goes through quoteNames, the unknown names go through quoteNames, and the ambiguous clause uses %q for the entry, while src.Name was already %q. [T] Red before: TestCheckClientRxSourcesQuotesConfigValues fails on origin/master in both subtests and passes on the merged tree. Other client-RX lines with config values via %s: the drop warnings at client_rx_sources.go:84/88 (F1). The MQTT [%s] tag convention is ingestor-wide, so I'd fix it centrally in a follow-up (sanitise or %q the tag in mqttSourceTag/mqttSourceTags, or reject control characters in mqttSources[].name at config load) rather than in this PR. Related but not config-derived: main.go:706 (client %.8s blacklisted) prints the publisher-controlled topic segment parts[2] with %.8s, which can carry a newline within its 8 runes. It is pre-existing and outside Follow-ups to #290: quote config values in client-RX source warnings, pin message details, consistent disabled-coverage wording #302 [A].
2. N2 (two survivors): killed. [T] Against the merged tree, strings.Join(ambiguous, "; ") → " " is killed by TestCheckClientRxSourcesAmbiguousMessageShape. So is len(matched) → 2, because the 3-source entry makes the count matter.
3. N3 (disabled wording): consistent and pinned. [A] When enabled: false, the code switches to would be accepted / would never be accepted. Otherwise it keeps the present tense. [T] The disabled subtest is red before and green after. The enabled subtest is green before and after, which is correct: it pins the unchanged direction and kills an "always conditional" mutant (M7 below).
4. N4 (case-insensitive dedup): done with a test. [A] dedupeNamesFold runs on the already-trimmed allowlist (ClientRxCoverageSources trims), keeps the first spelling, and is O(n) with one small map. [T] TestCheckClientRxSourcesDeduplicatesAllowlist is red before in both subtests and green after. F2 covers the Unicode edge.
5. Scope: holds. [K] Two files changed, both in cmd/ingestor (client_rx_sources.go, new client_rx_sources_302_test.go). [A] The only behaviour change besides the log text is the deduplicated unknown return value, and its only caller ignores it. ClientRxCoverageSources() and ClientRxSourceAllowed() are untouched, so admission is unchanged.

Tests [T][K]

Merged tree = git merge-tree --write-tree origin/master f9e3c49c… on origin/master bf3151a4 (clean, same 2-file diff), extracted with git archive into scratch.

What	Result
Red before: new _302_test.go on origin/master code	4/4 tests FAIL (N1 ×2 subtests, N2, N3 disabled, N4 ×2 subtests); N3 enabled passes, as intended
cmd/ingestor: go vet ./... && go test -count=1 ./... (merged)	ok (2152 s; slow because it shared the CPU with my mutant and E2E runs), vet clean
cmd/server: go vet ./... && go test -count=1 ./... (merged)	ok (1351 s), vet clean
sh test-all.sh (merged)	225 passed, 0 failed (225 files)
node test-frontend-helpers.js (merged)	709 passed, 0 failed
gofmt -l on both touched files	clean

E2E [T]: I ran a local Go server built from the merged tree on a scratch copy of e2e-fixture.db, prepared as in CI: tools/freshen-fixture.sh, then the inline Kpa-clawbot#1486/Kpa-clawbot#1791 seed SQL from deploy.yml, then corescope-migrate, then seeds 2073, 199 and 245. I used plain public/ and the default config, so coverage is off, as in CI. The PR has no E2E of its own. The affected ones are the client-RX/reach suites, and all passed or skipped as expected: test-node-reach-e2e.js OK, test-reach-rank-e2e.js 14/0, test-issue-1630-reach-mobile-e2e.js 7/0, test-node-reach-coverage-e2e.js and test-rx-coverage-mobile-nav-e2e.js SKIP (coverage disabled), test-issue-199-inactive-observer-e2e.js 3/0, test-issue-2073-recent-adverts-e2e.js 10/10. I stopped the server by the pid on its listening port and checked that the port was free afterwards [K].

Mutants (mine, each applied alone to the merged tree, run against TestCheckClientRxSources|TestClientRxSource|TestClientRxCoverage|TestLoadConfigClientRx) [T]:

Mutant	Result
M1 strings.Join(ambiguous, "; ") → " " (#290 survivor)	killed: AmbiguousMessageShape
M2 len(matched) → 2 (#290 survivor)	killed: AmbiguousMessageShape
M3 dedup key strings.ToLower(name) → name (case-sensitive)	killed: DeduplicatesAllowlist (both subtests)
M4 dedup keeps the last spelling	killed: DeduplicatesAllowlist (both subtests)
M5 dedup early-return len(names) < 2 → < 3	killed: DeduplicatesAllowlist/unknown_name_once
M6 disabled-branch moods swapped	killed: DisabledWordingAgrees/disabled
M7 conditional mood always on	killed: DisabledWordingAgrees/enabled
M8 unknown names unquoted	killed: QuotesConfigValues/state_line_and_unknown
M9 state line unquoted	killed: QuotesConfigValues (both), DeduplicatesAllowlist
M10 ambiguous entry %q → %s	killed: QuotesConfigValues/ambiguous, AmbiguousMessageShape
M11 ambiguous entry %q → %v	killed: QuotesConfigValues/ambiguous, AmbiguousMessageShape

11/11 killed, no survivors. After restoring the file, the filtered set is green again.

CI [K]: run 37472108114, attempt 1 (only run for this branch), head f9e3c49c merged into de6e83b4, which is an ancestor of the current origin/master. Go Build & Test: success. Playwright E2E: success. Build & Publish Docker Image: success. Release Artifacts, Deploy Staging and Publish Badges & Summary: skipped by their guards. The known flakes #256 (Hash Stats sort) and #267 (backfill write-hold) did not show up. The two origin/master commits since that CI base are covered by my merged-tree runs above.

Always-checks [K]

• Only cmd/ingestor touched. cmd/server, cmd/migrate, public/ and .github are identical to origin/master, so the server stays read-only.
• 0 added lines containing map[string]interface{} (tests included).
• No hardcoded colours: no hex-colour pattern on added lines other than issue references.
• bash scripts/check-xss-sinks.sh --diff origin/master exits 0 with "no public/**/*.{js,html} changes to scan".
• Fork guards: github.repository == 'Kpa-clawbot/CoreScope' appears as an if: guard 9× in deploy.yml (plus one comment line) and 1× in release-fast-path.yml.
• No closing keywords in the title, body or either commit message. The body opens with "Relates to Follow-ups to #290: quote config values in client-RX source warnings, pin message details, consistent disabled-coverage wording #302".
• Both commits (29ac6234, f9e3c49c) are authored and committed by dborup <kontakt@meshview.dk>.
• Head f9e3c49c was unchanged on the remote before and after this review (git ls-remote).

Not verified

• main() has no automated test. I did not repeat the author's real-boot before/after run; the call site (main.go:79) was checked by reading only.
• No live MQTT broker. The drop-warning forgery (F1) is shown with a unit-level probe, not on a running ingestor.
• E2E ran against plain public/, not the coverage-instrumented copy CI builds, and only the client-RX/reach suites plus the 199/2073 seed suites ran, not the full Playwright list. No coverage-enabled E2E: as in feat(ingestor): accept client RX coverage only from configured sources (#265) #274/test(ingestor): pin clientRxCoverage.sources through LoadConfig (#278) #290, the fixture lacks the ingestor-created client_* tables.
• No browser or visual validation (no UI change), no staging or production access, and no server API key used.