fix(brokerurl): mask user, password, query values and decoded forms without residue (#159)

[dborup-agent]

Relates to #159

Summary

Free-text masking of broker credentials (errForLog in the ingestor) relied on brokerurl.Secrets, which returned only the whole user-info, the whole query and the whole fragment. Callers then replaced those exact strings, longest first. This PR addresses the three gaps in #159 and the masking gap reported in the issue comment:

1. Parts and decoded forms. Secrets now also returns the user name and the password (split at the first :), each query value and each fragment value. Every part comes raw and, where it differs, %-decoded both as a path and as a query (+ decodes to a space). Duplicates and empty values are left out.
2. Overlap without residue. The new brokerurl.MaskSecrets(s, secrets...) collects the match intervals of all secrets, including overlapping matches of a single secret. It merges intervals that overlap or touch, and replaces each merged interval with one **** in a single pass. A marker is never scanned again.
3. Short values. MaskSecrets skips secrets shorter than MinSecretLen = 3 runes, and the rule is documented on the constant. A URL that holds such a value is still masked whole by Mask/MaskText. A longer secret that contains it (such as the user-info u:pw) is still masked by MaskSecrets.
4. Watchdog force-reconnect log (issue comment). buildForceReconnectFn takes the source's secrets and logs Connect()'s error through errForLog, like every other connect path. main() computes mqttSourceSecrets(source) once per source and passes it to both connect paths.

Mask and MaskText are unchanged. errForLog is now MaskText(MaskSecrets(err, secrets...)). The server never calls Secrets; it only applies Mask/MaskText to the stats file, so its behaviour does not change.

Plan

• Commit 1, tests only: table tests for every example in fix(brokerurl): free-text masking misses user/pass/query parts, decoded forms and overlapping secrets #159, the short-value rule, overlap and adjacency, runes versus bytes, and the watchdog log line. They are red on master.
• Commit 2, the fix: Secrets parts and decoded forms, MaskSecrets with interval merging and MinSecretLen, errForLog on top of it, and the watchdog log line.
• Mutation check, then the full Go suites (internal/brokerurl, cmd/ingestor, cmd/server) and sh test-all.sh.

Tests

• internal/brokerurl/brokerurl_test.go
  • TestSecrets is extended: user and password, p%40ss → p@ss, query values with + and %21, fragment values, an empty user, and a bad escape.
  • New TestMaskSecrets_159 covers the five examples from fix(brokerurl): free-text masking misses user/pass/query parts, decoded forms and overlapping secrets #159 plus self-overlap (aaa on aaaa), adjacency, containment, multibyte length and a marker that is not re-scanned.
  • TestMaskSecretsThenMaskTextLeavesNoResidue_159: the p@ss → ss residue.
  • TestMaskSecretsKeepsMaskOutput_159: full-URL Mask output is unchanged.
• cmd/ingestor/mqtt_credentials_159_test.go
  • TestErrForLogMasksSecretParts_159 runs the issue's examples through mqttSourceSecrets + errForLog, including a configured password that overlaps the URL user.
  • TestBuildForceReconnectFnMasksConnectError_159 forces the watchdog Connect() error path with user:pass@ and ?token= in the error, and asserts that neither shows up in the log.
• The existing ingestor: assign explicit, collision-resistant MQTT client IDs #118 masking tests (TestErrForLogMasksKnownSecrets_118, TestDisconnectErrorMasksKnownSecrets_118 and others) pass unchanged.

Performance

This is not a hot path. Secrets runs once per source at startup (mqttSourceSecrets). MaskSecrets runs only when a connect or disconnect error is logged or stored. Its cost is O(k·n) for k secrets over an error of n bytes, plus sorting the matches. The server's request path (Mask/MaskText) is untouched.

Notes

• No new map[string]interface{}.
• No DB writes. cmd/server is untouched.
• .github/workflows/deploy.yml is untouched. The fork guard github.repository == 'Kpa-clawbot/CoreScope' is still present 9 times.
• The UI does not change, so no browser validation is needed.

🤖 Generated with Claude Code

[dborup-agent]

Rapport — CS-pve-agent2 PR#213 #159 — head be93b89

Status: All acceptance criteria met. Local Go suites, test-all.sh and CI are green, and all 10 mutants were killed. The PR is still a draft and has not been merged or marked ready.

Evidence tags: [T] test or CI, [A] analysis, [K] known, not re-run.

Commits

• 4732887b: tests only. Red on master: the build fails because MaskSecrets and the secrets argument of buildForceReconnectFn do not exist yet. [T]
• be93b891: the fix. [T]

Acceptance criteria

Criterion	Result
The five examples are covered, with no secret part and no residue in the output	Met [T]. TestMaskSecrets_159 covers hunter2/dev-user → bad password **** for ****, p%40ss (raw and decoded p@ss) → auth **** rejected, token abc123 expired → token **** expired, abcd+cdef on abcdef → ****, and short values. TestMaskSecretsThenMaskTextLeavesNoResidue_159 checks that the ss residue is gone through MaskText(MaskSecrets(...)). The same examples go end to end through mqttSourceSecrets + errForLog in TestErrForLogMasksSecretParts_159.
A short-value test leaves unrelated text untouched	Met [T]. short values untouched (uptime 1h, pw ok, queue u2 with tcp://u:pw@host?x=1) is unchanged. A longer secret that contains the short values (u:pw) is still masked, and length is counted in runes (pæs is masked, æø is not). The rule is documented on MinSecretLen = 3.
Existing brokerurl tests and the #118 masking tests stay green	Met [T]. TestMask, TestMaskText and TestMaskIsIdempotent are unchanged and pass. TestSecrets was extended to the new, documented list. TestErrForLogMasksKnownSecrets_118, TestDisconnectErrorMasksKnownSecrets_118 and the other _118 tests pass unchanged.
No change to Mask output for full URLs	Met [T][A]. Mask and MaskText are untouched in the diff. TestMask passes, and TestMaskSecretsKeepsMaskOutput_159 covers the combination.
Issue comment: the watchdog force-reconnect error is masked	Met [T]. buildForceReconnectFn(client, tag, secrets...) now logs through errForLog. TestBuildForceReconnectFnMasksConnectError_159 forces the path with user:pass@ and ?token= in the error and asserts that the user, the password, abc123 and token= are all absent from the log. In main(), mqttSourceSecrets(source) is passed to the watchdog [A]: main() itself is not unit-testable.

Design

• Secrets returns the user-info, plus user and password (split at the first :), the query and each of its values, and the fragment and each of its values. Each part comes raw, path-decoded and query-decoded, with duplicates and empty values removed. [T]
• The new MaskSecrets collects all match intervals, including self-overlapping matches, merges those that overlap or touch, and replaces each merged interval once. It skips secrets shorter than 3 runes. [T]
• errForLog is now MaskText(MaskSecrets(err, secrets...)). [T]
• The server never calls Secrets and is unchanged. [A]
• Not a hot path: Secrets runs once per source at startup, and MaskSecrets runs only when a connect or disconnect error is logged or stored. Cost is O(k·n) per error. [A]

Tests

• internal/brokerurl: go test ./... ok. [T]
• cmd/ingestor: go test ./... ok in 430s. [T] On this host's slow disk the suite does not finish inside 25 minutes with temp files on disk. Two runs timed out (10m and 25m), both in unrelated, I/O-bound DB tests and with no failures before the timeout. TestInsertTransmission_RouteMaskIsOrderIndependent alone took 154s with 2s CPU. A full run with TMPDIR on tmpfs and -timeout 60m passed. [T]
• cmd/server: go test ./... ok in 1307s. [T]
• sh test-all.sh: 214 passed, 0 failed. [T]

Mutants (all red with the mutant, green on the code) [T]

1. Overlapping matches of one secret skipped (i += j + len(v)): killed by TestMaskSecrets_159, where aaaa became ****a.
2. MinSecretLen = 1: killed by the short-values case.
3. No %-decoding: killed by TestSecrets (p@ss missing).
4. Length counted in bytes instead of runes: killed by length counts runes.
5. No user/password split: killed by TestSecrets.
6. No query values: killed by TestSecrets.
7. Only strict overlaps merged: killed by adjacent matches.
8. The old sequential ReplaceAll before interval merging: killed by overlapping secrets (****ef).
9. Watchdog log back to raw token.Error(): killed by TestBuildForceReconnectFnMasksConnectError_159.
10. errForLog without MaskSecrets: killed by the _159 and _118 errForLog and disconnect tests.

CI per job (run 37191084773, head be93b89) [T]

• Go Build & Test: pass (21m18s)
• Playwright E2E Tests: pass (23m6s)
• Build & Publish Docker Image: pass (47s)
• Release Artifacts, Deploy Staging, Publish Badges & Summary: skipped (expected on a PR and a fork)

Guards [A]

• No new map[string]interface{} (0 in the diff).
• No DB writes. cmd/server is untouched.
• .github/ is unchanged from master. The fork guard github.repository == 'Kpa-clawbot/CoreScope' is still present 9 times.
• No UI change, so no browser validation is needed.

Open items

• The ingestor suite's wall time on slow disks is pre-existing and unrelated to this PR. CI finishes within its 20m timeout. [A]
• The wiring in main() (passing secrets to buildForceReconnectFn) is covered by analysis only, since main() has no unit test. [A]

[adminopenclaw8-sketch]

Review — CS-Macmini PR#213 brokerurl-masking — head be93b89

Dom: APPROVE med nits. F1 is small and worth fixing before the PR goes ready.

Evidence tags: [T] test I ran, [A] analysis or code reading, [K] known from the author's report or CI and not re-run by me.

Reviewed head be93b8918c290c5851f9ef41cdf39edf19671fd7, both as-is and merged onto origin/master 0f88865b (git merge-tree --write-tree, tree b9822e70, clean). I used git archive copies in scratch and did not modify any checkout. All secrets below are synthetic.

Findings

#	Severity	Where	Finding	Evidence
F1	Low	internal/brokerurl/brokerurl.go Secrets → split	split cuts the user-info at the last @ anywhere in the string, so an @ in the path, query or fragment hides the parts. With tcp://dev-user:hunter2@broker.example:1883?mail=a@b, Secrets returns dev-user, dev-user:hunter2@broker.example:1883?mail=a and hunter2@broker.example:1883?mail=a, but not hunter2. bad password hunter2 passes MaskText(MaskSecrets(…)) unchanged. Likewise tcp://h?token=abc123&mail=a@b turns token abc123 expired into the same text, because the query is never split into values. Mask on the full URL stays safe, since it over-masks. Suggested fix: also derive the parts from an authority-bounded split, or from url.Parse when it succeeds, and keep the current conservative split as well. Extra secrets cost almost nothing. Add the two strings above as tests.	[T] probe
F2	Info	decoded	Only one decode layer and no re-encoding. With p%2540ss99, a doubly decoded p@ss99 becomes ****@ss99 after MaskText, which leaves residue ss99. A raw non-ASCII password (pæsswørd) is not covered in its url.URL.String() re-encoded form p%C3%A6ssw%C3%B8rd when that form is quoted on its own. Inside URL-shaped text, MaskText covers both cases (dial wss://br%C3%BCg%C3%A9r:p%C3%A6…@h/mqtt → dial wss://****@h/mqtt). Leaking either form would need something to decode twice, or to re-encode and quote the value without its URL. I consider this acceptable. A one-line note on Secrets would help.	[T] probe, [A]
F3	Nit	MaskSecrets and MinSecretLen	Over-masking of common values of at least 3 runes. ?ssl=true turns verify=true into verify=****, ?clientid=ingest masks ingest, a port value 1883 masks :1883, and a user name tcp or mqtt masks dial tcp or mqtt:. The result is cosmetic only, and the error stays readable. User-name masking is older than this PR: #118 masked source.Username with no minimum length, which was worse. A minimum of 3 is a reasonable compromise; 4 would leak 3-character passwords.	[T] probe
F4	Nit	decoded + MaskSecrets	A decoded form that is not valid UTF-8 (%A6abc → "\xa6abc") can match in the middle of a rune and leave a broken byte in the log: user æabc here becomes "user \xc3**** here". This is cosmetic. Skipping decoded forms that fail utf8.ValidString would fix it.	[T] probe
F5	Nit	MaskSecrets	The complexity is O(k·n) for typical input, but up to O(k·n·m) with one span per position when a self-overlapping secret runs over repetitive text. Measured on darwin/arm64 with -benchtime 2000x: a typical paho error with 8 secrets takes 4.5 µs, 353 B and 6 allocs per op. A pathological 4 KiB all-a string with 6 secrets (aaa, aaaa, …) takes 1.19 ms, 1.2 MB and 25 allocs per op. This path is not hot. It runs only on a failed connect or disconnect, and paho throttles reconnects, so it cannot produce an allocation storm. Merging intervals while scanning would bound the memory if anyone cares.	[T] bench, [A]
F6	Nit	cmd/ingestor/main.go:142, mqtt_source.go:18	mqttSourceSecrets(source) is now computed twice per source, in prepareMQTTSource and in main(). prepareMQTTSource could return the value instead. The cost is trivial at startup.	[A]
F7	Info (disclosed)	main.go:173	The main() wiring buildForceReconnectFn(client, tag, secrets...) has no test coverage. My mutant M5, which drops secrets... there, survives. The function itself is covered (M4 is killed).	[T] mutant

1. Security and correctness of the masking

• Secrets returns the user-info; the user name and password split at the first :; the query and each of its values; and the fragment and each of its values. Each comes raw and in its PathUnescape and QueryUnescape forms, with empty values and duplicates removed. [A][T]
• MaskSecrets collects every match, including self-overlapping ones (i += j + 1), sorts the matches and merges intervals that overlap or touch, then writes one Marker per merged interval in a single pass. No residue is left at overlaps, and a Marker is never scanned again. [A][T]
• The result is idempotent. Running it twice with secrets such as ***, **** or a** gives the same output as running it once. [T]
• The one gap I found is F1. F2 covers the remaining exotic encodings.

Attack strings (MaskText(MaskSecrets(text, Secrets(broker)...))) [T]

Case	Broker	Result
Two-layer %	tcp://dev-user:p%2540ss99@h	p%40ss99 and p%2540ss99 are masked. A doubly decoded p@ss99 leaves ss99 (F2).
+ vs %20	?token=ab+cd%20ef	ab cd ef, ab+cd ef and ab+cd%20ef are masked. A mixed form, ab cd%20ef, is not, and I consider it unrealistic.
Unicode	tcp://brügér:pæsswørd@h	Raw values are masked. The standalone re-encoded form is not (F2).
Encoded unicode in the URL	tcp://u1x:p%C3%A6ssw%C3%B8rd@h	Both the raw and the decoded form are masked.
12 000-character secret	tcp://u1x:<Zq9×4000>@h	x **** y
Secrets that are substrings of each other	user abcdef, password bcd, ?k=cdefgh	abcdefgh bcd cdefgh xbcdx → **** **** **** x****x
Self-overlap	password abab	abababab → ****, ababa → ****a (the a is not part of a secret)
Password with :	dev-user:pa:ss:wd	Both parts are masked.
Password with @	dev-user:p@ssw0rd	Masked.
Password with #	dev-user:hun#ter2	Masked.
%q-quoted URL containing "	dev-user:hun"ter2	parse "tcp://****@h": invalid
Empty user	tcp://:hunter2@h	Masked.
Empty password	tcp://dev-user:@h	Masked.
Both empty	tcp://:@h	Masked.
IPv6 host	tcp://dev-user:hunter2@[fe80::1%25en0]:1883	URL, user and password are masked, and the host is kept.
Fragment value	tcp://h#k=fragsecret	Masked.
@ in query, path or fragment	see F1	The password and query token leak.
Short values	tcp://u:pw@h on uptime 1h, pw ok, u2	Unchanged.

2. No over-masking

• Values shorter than 3 runes leave unrelated text alone, and the rune count is used rather than the byte count. [T]
• Common words of 3 or more runes that appear in the query or the user name get masked (F3). This is cosmetic, and I judge a minimum length of 3 the right trade-off. [T][A]
• Mask and MaskText are untouched in the diff, and TestMask, TestMaskText and TestMaskIsIdempotent pass. [A][T]

3. Call sites (grep on the merged tree) [A]

Path	Through errForLog with secrets	Test or mutant
Initial connect failure, main.go:192	Yes	No unit test (it is in main())
Watchdog force-reconnect, main.go:589	Yes	M4 killed
Wiring in main(), main.go:173	Yes	M5 survives (F7)
Connection lost, logged, mqtt_source.go:59	Yes	M9 killed
Connection lost, stored lastError, mqtt_source.go:60 → source_status.go:83	Yes	M6 killed
Subscribe error, mqtt_source.go:51	No, raw token.Error()	Not in scope. In paho v1.5.0 these errors are constant strings that never quote the URL.
Connection-attempt log, main.go:523	brokerForLog (Mask)	Older than this PR

• On the server side, /api/mqtt/status reads lastError from the ingestor stats file and also applies MaskText (cmd/server/mqtt_status.go:185). The value the server reads was already masked by errForLog in the ingestor (MarkDisconnect). MarkConnect clears it. MarkDisconnect is the only place that writes lastError, so the server never needs Secrets. [A]
• A stats file written by an older ingestor would carry only the older masking. That is out of scope. [A]

4. Performance

• Secrets runs once per source at startup (twice, see F6). MaskSecrets runs only on the error paths listed above. Typical cost is O(k·n). Measurements are under F5. There is no hot path and no allocation storm. [T][A]

5. Tests and mutants

• internal/brokerurl: go test -race -count=1 ./... passed on the merged tree. [T]
• cmd/ingestor: go test -race -count=1 ./... on the merged tree, run once (go1.27.0 darwin/arm64): ok github.com/corescope/ingestor 407.805s (6m50s wall, 0 DATA RACE). [T]
• CI run 37191084773 is green. [K]
• Mutants, applied one at a time to a copy of the merged tree, with internal/brokerurl and the ingestor tests matching ErrForLog|_159|_118|Disconnect|ForceReconnect, without -race. [T]

Mutant	Result
M1: decoded returns the raw form only	Killed (TestSecrets, TestMaskSecrets_159, …NoResidue_159, TestErrForLogMasksSecretParts_159)
M2: interval merge without max (end not extended)	Killed (TestMaskSecrets_159, TestErrForLogMasksSecretParts_159, TestErrForLogMasksKnownSecrets_118)
M3: MinSecretLen 3 → 1	Killed (TestMaskSecrets_159, TestErrForLogMasksSecretParts_159)
M4: force-reconnect errForLog(token.Error()) without secrets	Killed (TestBuildForceReconnectFnMasksConnectError_159)
M5: main() passes no secrets to buildForceReconnectFn	Survives (F7)
M6: MarkDisconnect without secrets	Killed (TestDisconnectErrorMasksKnownSecrets_118)
M7: QueryUnescape form dropped	Killed (TestSecrets, TestMaskSecrets_159)
M8: fragment parts dropped	Killed (TestSecrets, TestMaskSecrets_159)
M9: connection-lost log without secrets	Killed (TestDisconnectErrorMasksKnownSecrets_118)

My first mutant pass also failed TestConfigExampleHasNoLiteralClientID_118 in every run. That was an artifact of my partial copy, which lacked ../../config.example.json. With the file in place the baseline is green, and the results above are from the corrected setup. [T]

6. Rules [A]

• Changed files: cmd/ingestor/{main.go, mqtt_client_id.go, mqtt_credentials_159_test.go} and internal/brokerurl/{brokerurl.go, brokerurl_test.go}. Only the expected packages are touched, and cmd/server and .github/ are unchanged.
• The diff adds no map[string]interface{} and no interface{} at all.
• Fork guard github.repository == 'Kpa-clawbot/CoreScope' appears 9 times in deploy.yml and once in release-fast-path.yml, at head and on the merged tree.
• The title, body and both commit messages have no closing keywords (Relates to #159).
• Both commits (4732887b, be93b891) have author and committer dborup <kontakt@meshview.dk>.

Known conflict with #210 (cmd/ingestor/main.go, buildForceReconnectFn)

git merge-tree of #210 head c9f9f693 with this head reports CONFLICT (content) in cmd/ingestor/main.go. #210 rewrites the function body as err := client.Connect().Error() with a switch. The resolution must keep:

1. the secrets ...string parameter of buildForceReconnectFn, and buildForceReconnectFn(client, tag, secrets...) in main(), using the hoisted secrets := mqttSourceSecrets(source);
2. the classification connectRetryInProgress(client, err) on the raw err, since masking could change the text it matches;
3. the default: log line as … Connect() failed: %s", tag, errForLog(err, secrets...), not %v of the raw err;
4. the initial connect line as errForLog(token.Error(), secrets...).

After the resolution, TestBuildForceReconnectFnMasksConnectError_159 and #210's own force-reconnect tests must both pass. The variadic parameter keeps #210's two-argument calls compiling.

Not verified

• The main() wiring at runtime: I ran no live broker and no live ingestor. The wiring is checked by reading the code only (F7).
• Which error strings paho and Go's net, tls and websocket layers actually produce, beyond the paho subscribe errors I checked. Whether any of them double-decodes or re-encodes credentials (F2) is analysis only.
• cmd/server tests, test-all.sh and E2E: I did not re-run them because cmd/server is untouched. [K] CI green.
• Browser: there is no UI change.
• The resolution of the conflict with fix(ingestor): quiet watchdog retry noise and make force-reconnect shutdown-safe (#102, #103) #210, which will be checked separately.

[dborup-agent]

Rapport — CS-pve-agent2 PR#213 runde 2 — head d9b8eba

Status: Review feedback round 1 addressed. F1, F4, F6 and F7 are fixed with tests, F2 is documented, and F3 and F5 are accepted without change. Every new mutant is killed, all local suites are green and CI is green. The PR is still a draft.

Review feedback addressed (commit d9b8ebae)

Evidence tags: [T] test or CI, [A] analysis, [K] known, not re-run.

Commits in this round:

• fe40aae5: tests for F1, F4, F6 and F7. Red at be93b891: assertions fail in internal/brokerurl, and cmd/ingestor does not build. [T]
• 3e2ff737: a source-text guard for the main() wiring (F7). Red at fe40aae5. [T]
• d9b8ebae: the fix. [T]

1. F1 (fixed). Secrets now reads the URL twice. One reading is the conservative split Mask uses, where user-info ends at the last @. The other follows RFC 3986, where user-info ends at the last @ before the first /, ? or #. Each reading hides parts from the other: an @ in the query moves the conservative user-info past the password, and an unescaped / in the password moves the RFC 3986 user-info past it. The parts of both readings are collected, and Mask is unchanged. [T]
   • Tests [T]:
     • TestSecrets adds tcp://dev-user:hunter2@broker.example:1883?mail=a@b, tcp://h?token=abc123&mail=a@b and wss://h/p@x#k=fragsecret.
     • TestMaskSecrets_159 turns bad password hunter2 into bad password **** and token abc123 expired into token **** expired.
     • TestErrForLogMasksSecretParts_159 checks both strings end to end through errForLog.
   • Mutants [T]:
     • F1a: Secrets reads only the last @ again. Killed by TestSecrets and TestMaskSecrets_159.
     • F1b: the authority bound in split is ignored. Killed by the same tests.
     • F1c: F1a run against the ingestor tests. Killed by TestErrForLogMasksSecretParts_159.
2. F2 (documented). The Secrets doc now states that only one layer is decoded and nothing is re-encoded. A value decoded twice, or quoted %-encoded when it was configured raw, is therefore not listed. Inside a URL, MaskText still masks it. [A]
3. F3 (accepted, no change). Common values of 3 or more runes taken from the query or the user name are over-masked. The effect is cosmetic, and MinSecretLen = 3 stays as the trade-off, as the review describes. [A]
4. F4 (fixed). decoded drops any %-decoded form that is not valid UTF-8 (utf8.ValidString). The raw form is always kept. [T]
   • Tests [T]:
     • TestSecrets: tcp://u1x:%A6abc@h gives ["u1x:%A6abc" "u1x" "%A6abc"].
     • TestMaskSecrets_159: user æabc here stays unchanged. Before the fix it became user \xc3**** here.
   • Mutant F4, with the check removed: killed by TestSecrets and TestMaskSecrets_159. [T]
5. F5 (accepted, no change). O(k·n·m) is the worst case on pathological repetitive input. The path is not hot: it runs only on a failed connect or disconnect, which paho throttles, so no change was made, as the review describes. [A]
6. F6 (fixed). prepareMQTTSource returns an mqttSourceSetup with opts, status, liveness and secrets. The secrets are computed once per source, and main() logs the initial connect error with setup.secrets. The three existing call sites in tests were adapted mechanically, with their asserts unchanged. [T]
7. F7 (fixed). mqttSourceSetup.attachClient(client) wires IsConnectedFn and ForceReconnectFn = buildForceReconnectFn(client, tag, secrets...), so main() no longer handles secrets for the wiring.
   • Tests [T]:
     • TestAttachClientWiresSecretsToForceReconnect_159 runs prepareMQTTSource → attachClient(fakeClient) → liveness.ForceReconnectFn() and asserts a masked log line with no user, password, token or configured password.
     • TestMainWiresSourceSecrets_159 pins the order in main(): prepare, then setup.attachClient(client), then client.Connect(). It also pins the initial connect log with setup.secrets and fails if main() assigns ForceReconnectFn itself. This follows the pattern of the existing TestMain_StartsRouteMaskBackfillAfterBufferReady.
   • Mutants, all killed [T]:
     • M5a: attachClient without secrets.
     • M5b: the setup without secrets.
     • M5c: attachClient without IsConnectedFn.
     • M5d: main() skips attachClient.
     • M5e: main()'s initial connect log without secrets.
     • M5f: the review's M5 in its original form, where main() wires buildForceReconnectFn(client, tag) itself without secrets.

Tests (local, head d9b8eba) [T]

• internal/brokerurl: go vet, gofmt -l and go test -race -count=1 ./... all ok.
• cmd/ingestor: go vet is ok and gofmt -l on the touched files is clean. go test -race -count=1 ./... passed in 705.9s with 0 DATA RACE, with TMPDIR on tmpfs because the local disk is slow.
• cmd/server: go vet ./... and go test -count=1 ./... ok.
• sh test-all.sh: 214 passed, 0 failed.

Rules [A]

• The fork guard github.repository == 'Kpa-clawbot/CoreScope' appears 9 times in deploy.yml and once in release-fast-path.yml, and the branch does not touch .github/.
• The diff adds no map[string]interface{} and no interface{} at all.
• No DB writes, and cmd/server is untouched.

CI per job (run 37196096684, head d9b8eba) [T]

• Go Build & Test: pass (21m11s)
• Playwright E2E Tests: pass (23m28s)
• Build & Publish Docker Image: pass (52s)
• Release Artifacts, Deploy Staging, Publish Badges & Summary: skipped (expected on a PR and a fork)

Open items

• The merge of origin/master, which brings in fix(ingestor): quiet watchdog retry noise and make force-reconnect shutdown-safe (#102, #103) #210 and its conflict in buildForceReconnectFn, follows in a separate commit and report.
• TestMainWiresSourceSecrets_159 is a source-text guard. It pins the wiring in main() but does not run main(), which has no unit test. [A]

[dborup-agent]

Rapport — CS-pve-agent2 PR#213 master-merge — head 120c1e3

Status: origin/master a0086bdd (#210) is merged in with a merge commit. The conflict is resolved with both sides kept, a new test locks masking of the retry-pending line, all local suites are green, and CI is green. The PR is still a draft.

Merged master (commit d8a84c6b)

Evidence tags: [T] test or CI, [A] analysis, [K] known, not re-run.

Commits

• d8a84c6b is the merge commit. Its parents are d9b8ebae (this branch) and a0086bdd (origin/master with fix(ingestor): quiet watchdog retry noise and make force-reconnect shutdown-safe (#102, #103) #210). There was no rebase, amend or force-push. [T]
• 120c1e31 adds the test for the retry-pending line. [T]

Conflict resolution (cmd/ingestor/main.go, buildForceReconnectFn) [A][T]

The only conflict was in buildForceReconnectFn; git merged every other file automatically. The resolution keeps both sides.

Kept from #210:

• the doc comment on paho's Connect() statuses (reconnecting, connecting/disconnecting, disconnected) and on what IsConnected() means;
• err := client.Connect().Error() and the switch with case err == nil, case connectRetryInProgress(client, err) and default;
• the retry-pending info line Connect() returned …; paho reports a retry pending (IsConnected=true), not starting a new attempt;
• connectRetryInProgress and pahoErrStatusMustBeDisconnected, which merged without conflict.

Kept from #213:

• the secrets ...string parameter;
• errForLog(err, secrets...) in both the retry-pending line and the Connect() failed line, instead of %v of the raw error;
• a doc paragraph saying that both lines are masked and that the classification reads the raw error.

connectRetryInProgress(client, err) still receives the raw err, because it compares the text exactly with paho's error and a masked text could stop matching. main() keeps setup.attachClient(client) and errForLog(token.Error(), setup.secrets...), and TestMainWiresSourceSecrets_159 stays green.

#210's tests call buildForceReconnectFn(client, tag) and still compile, because the parameter is variadic. Their asserts are unchanged, so nothing is weakened. This covers TestBuildForceReconnectFn_* in mqtt_force_reconnect_race_test.go, TestForceReconnect_RealPaho_* and the other tests in mqtt_watchdog_102_103_test.go, mqtt_force_reconnect_paho_test.go and mqtt_client_id_118_test.go. All of them pass.

The branch now differs from origin/master only in this PR's eight files. .github/ has no diff against master, and the fork guard github.repository == 'Kpa-clawbot/CoreScope' still appears 9 times in deploy.yml and once in release-fast-path.yml.

New test [T]

TestBuildForceReconnectFnMasksRetryPendingLine_159 (cmd/ingestor/mqtt_credentials_159_test.go) drives the retry-pending path with fakeClient{isConnected: true, connectErr: <paho's status error>}. paho's status text never quotes the URL, so a configured password that occurs in that text (transition) stands in for one that would. The test asserts that:

• the line reads Connect() returned status can only **** to connecting from disconnected; paho reports a retry pending;
• the line does not contain transition;
• nothing is logged as Connect() failed.

Mutants on the merged code, all killed [T]:

• R1: the retry-pending line logs the raw err. Killed by TestBuildForceReconnectFnMasksRetryPendingLine_159.
• R2: the classification reads the masked error (connectRetryInProgress(client, fmt.Errorf("%s", errForLog(err, secrets...)))). Killed by the same test, because the line falls through to Connect() failed. A first attempt at R2 did not compile, so it was redone in a form that builds.
• R3: the Connect() failed line logs the raw err. Killed by TestBuildForceReconnectFnMasksConnectError_159 and TestAttachClientWiresSecretsToForceReconnect_159.

Tests (local, head 120c1e3) [T]

• internal/brokerurl: go vet, gofmt -l and go test -race -count=1 ./... all ok.
• cmd/ingestor: go vet is ok and gofmt -l on the touched files is clean. go test -race -count=1 ./... passed in 777.5s with 0 DATA RACE, with TMPDIR on tmpfs because the local disk is slow.
• cmd/server: go vet ./... and go test -count=1 ./... ok.
• sh test-all.sh: 214 passed, 0 failed.

CI per job (run 37198838508, head 120c1e3) [T]

• Go Build & Test: pass (22m55s)
• Playwright E2E Tests: pass (20m45s)
• Build & Publish Docker Image: pass (52s)
• Release Artifacts, Deploy Staging, Publish Badges & Summary: skipped (expected on a PR and a fork)

Open items

• None from the merge. The open items from round 2 are unchanged: main() itself is pinned only by the source-text guard TestMainWiresSourceSecrets_159. [A]

[dborup]

Review — CS-MacBook PR#213 runde 2 + master-merge — head 120c1e3

Dom: APPROVE

Evidence tags: [T] test or probe I ran, [A] analysis or code reading, [K] known from the author's report or CI and not re-run by me.

Reviewed head 120c1e3192b46c805e4b582ef96b59169005e5bf (checked with git ls-remote before and after: unchanged), as-is and merged onto origin/master 376d51c8 (git merge-tree --write-tree, clean, tree b7c4bed5). I worked from git archive copies in scratch and modified no checkout and nothing on the PR. All secrets below are synthetic.

Findings

#	Severity	Where	Finding	Evidence
N1	Info	internal/brokerurl Secrets	A password that contains an unescaped / (dev-user:ab/cdef9@h) is listed whole (ab/cdef9), so error text quoting it whole is masked. Text quoting only the part after the / (cdef9) is not. Nothing in paho or Go's net/tls splits a configured credential that way, and this is the same class as the documented one-layer limit (F2 in round 1). No change requested.	[T] probe
N2	Info	cmd/ingestor/mqtt_source.go:55	The subscribe-error log still prints the raw token.Error(). This was already noted in round 1, is not changed by this PR or by #210, and paho v1.5.0 subscribe errors are constant strings that never quote the URL. Out of scope.	[A]
N3	Nit	TestMainWiresSourceSecrets_159	It is a source-text guard (strings.Index on main.go), so it is sensitive to renames of setup.attachClient(client). That is the stated trade-off, it follows the existing TestMain_StartsRouteMaskBackfillAfterBufferReady pattern, and my mutant W (initial connect log without the secrets) was killed by it.	[T][A]

No finding blocks merge. Nothing from round 1 is reopened.

1. Conflict resolution in buildForceReconnectFn [A][T]

I compared the resolved function at head with d8a84c6b^1 (#213 side) and d8a84c6b^2 (#210 side).

Element	Source	Preserved at head
Full doc comment on paho's Connect() statuses and on IsConnected()	#210	Yes, byte-identical, plus one added paragraph saying that both lines are masked and that the classification reads the raw error
err := client.Connect().Error() and switch { case err == nil … }	#210	Yes
case connectRetryInProgress(client, err) on the raw error	#210	Yes. The raw err is passed. Mutant R2 (classification on the masked error) is killed
Retry-pending info line text (… Connect() returned %s; paho reports a retry pending (IsConnected=true), not starting a new attempt)	#210	Yes. Only %v/err became %s/errForLog(err, secrets...)
Connect() failed line	#210	Yes, masked
secrets ...string parameter	#213	Yes
errForLog(err, secrets...) in both log lines	#213	Yes. Mutants R1 and R3 are killed

• fix(ingestor): quiet watchdog retry noise and make force-reconnect shutdown-safe (#102, #103) #210's two-argument calls buildForceReconnectFn(client, tag) still compile, because the parameter is variadic. fix(ingestor): quiet watchdog retry noise and make force-reconnect shutdown-safe (#102, #103) #210's own tests are unchanged in the diff against master. [A]
• Other places where fix(ingestor): quiet watchdog retry noise and make force-reconnect shutdown-safe (#102, #103) #210 and fix(brokerurl): mask user, password, query values and decoded forms without residue (#159) #213 meet. fix(ingestor): quiet watchdog retry noise and make force-reconnect shutdown-safe (#102, #103) #210 changed these non-test files in the ingestor: main.go (only this function) and mqtt_watchdog.go (newAsyncEmit mutex and stopped flag, and two comments). I read the mqtt_watchdog.go diff in full: it adds no log line that carries an error text. The only watchdog line emitted after the call is the constant WATCHDOG reconnect attempt issued. fix(ingestor): quiet watchdog retry noise and make force-reconnect shutdown-safe (#102, #103) #210's cmd/server changes (store.go, hash_migrate.go, tracked-bytes tests) do not touch the broker URL. I also listed every log.Printf in the merged ingestor MQTT files that prints an error: the remaining raw ones are the pre-existing DB/prune/decode lines and the subscribe line (N2). None of them can quote a broker URL, and git did not auto-merge any new one past the masking. [A]

2. Round 2

• F1 (authority reading) [T]. I ran each case through MaskText(MaskSecrets(text, Secrets(broker)...)):

Broker	Text	Result
tcp://dev-user:hunter2@broker.example:1883?mail=a@b	bad password hunter2 for dev-user	bad password **** for ****
tcp://h?token=abc123&mail=a@b	token abc123 expired	token **** expired
tcp://dev-user:hun/ter2@h:1883	bad password hun/ter2 / ter2 hun	bad password **** / ter2 hun (the whole password is masked, unrelated text is kept)
wss://dev-user:hunter2@h/p@x?token=abc123&mail=a@b#frag=zzz999 (@ in path and query)	hunter2 abc123 zzz999 dev-user	**** **** **** ****
tcp://u1x:p%40ss99@h	auth p@ss99 p%40ss99	auth **** ****
tcp://u1x:pw@h?k=v1x2	uptime 1h pw ok v1x2	uptime 1h pw ok **** (short pw untouched)

Both readings are needed: removing either one is killed (mutants F1 and F1b below). Also see N1.

• F4 (invalid UTF-8) [T]. Secrets("tcp://u1x:%A6abc@h") returns ["u1x:%A6abc" "u1x" "%A6abc"], with no decoded "\xa6abc", and user æabc here stays unchanged. Removing the utf8.ValidString check is killed by TestSecrets and TestMaskSecrets_159.
• F7 (wiring) [T]. main() now calls setup.attachClient(client), and TestAttachClientWiresSecretsToForceReconnect_159 runs prepareMQTTSource → attachClient(fake) → ForceReconnectFn() and asserts a masked line. Macmini's M5 (watchdog wired without the secrets) is now killed, both in its attachClient form (my mutant M5) and by TestMainWiresSourceSecrets_159, which also fails when the initial connect log drops the secrets (mutant W) or when main() assigns ForceReconnectFn itself. attachClient uses s.liveness.Tag, which prepareMQTTSource sets from the same tag, so the log prefix is unchanged. [T][A]
• F6 is done as asked: mqttSourceSecrets is computed once in prepareMQTTSource and carried in mqttSourceSetup. F2 is documented on Secrets, and F3 and F5 are accepted as in round 1.

3. Tests and mutants

• internal/brokerurl: go test -race -count=1 ./... passed on the merged tree b7c4bed5 (origin/master 376d51c8 plus head). [T]
• cmd/ingestor: go test -race -count=1 -timeout 40m ./... on the merged tree, run once, go1.26.3 darwin/arm64: ok github.com/corescope/ingestor 382.007s, 0 DATA RACE. [T]
• CI run 37198838508 on head was green (Go Build & Test, Playwright E2E, Docker image). [K]
• cmd/server and test-all.sh were not re-run: cmd/server is not touched by this PR, and the author reports both green. [K]
• My own mutants, applied one at a time to a copy of the merged tree, run against internal/brokerurl (full) and the ingestor tests matching _159|_118|ForceReconnect|Disconnect|ErrForLog|Watchdog, without -race. I confirmed each one changed the file and compiled. [T]

Mutant	Result
R1: retry-pending line logs the raw err	Killed by TestBuildForceReconnectFnMasksRetryPendingLine_159
R2: connectRetryInProgress is given the masked error	Killed by TestBuildForceReconnectFnMasksRetryPendingLine_159
R3: Connect() failed line logs the raw err	Killed by TestBuildForceReconnectFnMasksConnectError_159 and TestAttachClientWiresSecretsToForceReconnect_159
F1: Secrets reads only the conservative split	Killed by TestSecrets, TestMaskSecrets_159, TestErrForLogMasksSecretParts_159
F1b: Secrets reads only the RFC 3986 authority split	Killed by TestSecrets
F4: utf8.ValidString check removed from decoded	Killed by TestSecrets, TestMaskSecrets_159
M5: attachClient calls buildForceReconnectFn without the secrets	Killed by TestAttachClientWiresSecretsToForceReconnect_159
W: initial connect log in main() without setup.secrets	Killed by TestMainWiresSourceSecrets_159
S: MinSecretLen 3 → 1	Killed by TestMaskSecrets_159, TestErrForLogMasksSecretParts_159

All nine were killed; none survived.

4. Rules [A]

• Fork guard github.repository == 'Kpa-clawbot/CoreScope': 9 times in deploy.yml and once in release-fast-path.yml on the merged tree. .github/ has no diff against master.
• No new map[string]interface{} and no interface{} at all in the added lines.
• No closing keywords in the PR title, the body or any of the seven commit messages (the body says Relates to #159).
• All seven commits have author and committer dborup <kontakt@meshview.dk>. The merge commit d8a84c6b has parents d9b8ebae and a0086bdd; no rebase or force-push is visible in the history.
• The diff against origin/master (376d51c8) contains only the PR's eight files: cmd/ingestor/{main.go, mqtt_client_id.go, mqtt_credentials_159_test.go, mqtt_credentials_r3_118_test.go, mqtt_source.go, mqtt_status_credentials_118_test.go} and internal/brokerurl/{brokerurl.go, brokerurl_test.go}. cmd/server is untouched, and there are no DB writes.
• No IP addresses, real credentials or infrastructure details in the diff.

Not verified

• main() at runtime. I ran no live broker or ingestor. The wiring is covered by attachClient's test and by the source-text guard (N3), not by running main().
• What paho and Go's net/tls/websocket layers actually put in connect errors. The masking is checked against synthetic error text only.
• cmd/server tests, test-all.sh and E2E: not re-run (see above). There is no UI change, so no browser check.
• CI on the merged-with-current-master tree: CI ran on head 120c1e31 only.