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: neither hunter2 nor dev-user is masked.
p%40ss in the URL: the error contains the decoded p@ss, and only part of it is masked, which leaves ss.
token abc123 expired for ?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
Secrets also 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
Summary
Follow-up from the round-3 review of #141 (head
3ff60504).internal/brokerurlmasks a broker URL well, but the free-text masking that relies onbrokerurl.Secretsmisses 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
abcdandcdef, the textabcdefbecomes****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
Secretsalso return the user name, the password, each query value and each fragment, both raw and URL-decoded.Mask.Acceptance criteria
brokerurltests and the fix(ingestor): assign explicit, collision-resistant MQTT client IDs #141 masking tests are still green.Maskoutput for full URLs.