Repository navigation
fix(brokerurl): free-text masking misses user/pass/query parts, decoded forms and overlapping secrets #159
Description
Summary
Follow-up from the round-3 review of #141 (head 3ff60504). internal/brokerurl masks a broker URL well, but the free-text masking that relies on brokerurl.Secrets misses secrets that appear in an error message without the URL around them, and it handles overlapping or very short secrets badly. Low severity: it only matters when a broker library or the OS echoes a credential back in an error string. This exists only on the #141 branch, so it is not on master yet.
Relates to #118.
Where (#141 head 3ff60504)
internal/brokerurl/brokerurl.go:40. Secrets(s) returns the whole user-info (user:pass), the whole query and the whole fragment. Callers mask those exact strings in free text.
Problems
1. Parts and decoded forms are not covered. Only the whole user:pass / query string is a secret. Found in the review:
bad password hunter2 for dev-user: neitherhunter2nordev-useris masked.p%40ssin the URL: the error contains the decodedp@ss, and only part of it is masked, which leavesss.token abc123 expiredfor?token=abc123: the query value alone is not masked.
2. Overlapping secrets leave a residue. With secrets abcd and cdef, the text abcdef becomes ****ef.
3. Very short values over-mask. A 1–2 character user name or password masks every occurrence of that substring in unrelated text.
Proposed fix
- Have
Secretsalso return the user name, the password, each query value and each fragment, both raw and URL-decoded. - Mask by merging the match intervals of all secrets, then replace each merged interval once, so that overlaps leave no residue.
- Skip values shorter than 3–4 characters for free-text matching. The URL itself is still masked by
Mask.
Acceptance criteria
- Unit tests cover the five examples above. None of the secret parts appear in the output, and no residue is left.
- A short-value test shows that unrelated text is untouched.
- Existing
brokerurltests and the fix(ingestor): assign explicit, collision-resistant MQTT client IDs #141 masking tests are still green. - No change to
Maskoutput for full URLs.
Activity
dborup commentedon Oct 2, 2026
Additional masking gap: the watchdog force-reconnect error is logged raw
Found during the 2026-10-02 staging round of master
727efca0+ #14141675d98+ #162baa68cdd(integration0b11401d). It was found by reading the code, not from a live log.Where:
41675d98:cmd/ingestor/main.go:578727efca0:cmd/ingestor/main.go:626Problem: this is the only connect-error log line that does not pass through
errForLog(cmd/ingestor/mqtt_client_id.go:184on #141). The other connect and reconnect paths mask the error. With #141 thetagis masked, buttoken.Error()is printed raw. Paho errors can echo the broker URL, including user-info or a query token.The line predates #141: it was added in
91d6d602on 2026-09-02. #141 did not introduce it.Impact today: none observed. No watchdog line fired during the round. Staging's three brokers carry their credentials in config fields, not in the URL, so nothing could leak there. The risk applies to an operator who puts credentials in the broker URL.
Fix: log
errForLog(token.Error(), <the source's secrets>), the same way as the other connect paths. Also add a test that forces this path with a broker URL containinguser:pass@and?token=, and asserts that neither appears in the log.Fits naturally with the
brokerurl.Secretswork in this issue.Merge pull request #213 from dborup/codex/issue-159-brokerurl-masking
dborup commentedon Oct 4, 2026
Fixed by #213 (merged as
7697a826).brokerurl.Secretsnow reads the broker URL twice: once with the conservative split and once with the RFC 3986 authority bound. It lists the user-info, user, password, query and fragment values, raw and in their single-decoded forms. Decoded forms that are not valid UTF-8 are skipped.MaskSecretsmerges overlapping and touching matches, so no residue is left. Values shorter than 3 runes are ignored.errForLogisMaskText(MaskSecrets(...))and is wired per source inmain().Connect() failed) are masked too. The classification still reads the raw error.Known limits, documented and accepted in review:
/is masked only as a whole.