test(ingestor): use non-numeric secrets in the #118 stats-file leak test (#250) by dborup-agent · Pull Request #253 · dborup/CoreScope

dborup-agent · 2026-10-05T11:47:31Z

Relates to #250

Problem

TestStatsFileHasNoCredentials_118 searches the whole stats-file JSON for each entry of secretParts118. That list contained the short secrets "1234", "2024" and "abc". Any counter, Unix time or size whose digits include 1234 therefore failed the test, although nothing had leaked. This was seen on #246.

Change (test code only)

mqtt_status_credentials_118_test.go: name the password and query parts and use them in the broker URLs, the error texts and secretParts118:
- credPassHead = "pw2024pw": a password cut at /
- credPassCut = "zq1234zq": a password cut at ? or #
- credQuery = "qxabcxq": the query or fragment part, and a token
mqtt_credentials_r3_118_test.go: these tests share secretParts118, so they now use credQuery for the token= value, including the log check that used to search for "abc".

Coverage is unchanged. The tests still check the masked user, the password cut at /, ? and #, the query and fragment, and the token. There is no production change.

Evidence (local)

Mutant: with errForLog returning err.Error(), 4 _118 tests fail, including TestStatsFileHasNoCredentials_118 (leaks dev-user, s3cret-pass, tok3n).
No dependence on numbers: a temporary, uncommitted test drove the real stats-file path with 1234 disconnects, so the file contained "disconnectCount":1234.
- With the old constants it failed with stats file leaks "1234", the test(ingestor): TestStatsFileHasNoCredentials_118 flakes when a stats number contains '1234' #250 symptom.
- With the new constants it passed.
go test -race -count=200 -run 'TestStatsFileHasNoCredentials_118$': ok.
cd cmd/ingestor && TMPDIR=<tmpfs> go test -race -count=1 -timeout 20m ./...: ok (705 s).
go vet ./...: ok. gofmt -l: both touched files are clean (other files listed by gofmt -l were already unformatted on master).

🤖 Generated with Claude Code

…ests (#250) TestStatsFileHasNoCredentials_118 searched the whole stats-file JSON for the test secrets "1234", "2024" and "abc". A counter, timestamp or size that contained "1234" failed it although nothing leaked (seen on #246). Name the password and query parts (pw2024pw, zq1234zq, qxabcxq) and use them in the broker URLs, error texts and secretParts118, and for the token in the r3 tests that share that list. Coverage is unchanged: the user, the password cut at '/', '?' and '#', the query/fragment part and the token are still checked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

dborup-agent · 2026-10-05T12:34:05Z

Rapport — CS-pve-agent2 PR#253 #250 — head `4e8f314`

Status: Test-only fix done and proven locally; Go Build & Test is green. The E2E job is red on both attempts, only from the #244 Details-clamp flake: this branch is based on master, which does not have that fix yet (PR #252).

Evidence tags: [T] tested here, [K] known from CI logs or code.

Change (test code only)

cmd/ingestor/mqtt_status_credentials_118_test.go: three named secret parts replace the short secrets "2024", "1234" and "abc":
- credPassHead = "pw2024pw": a password cut at /
- credPassCut = "zq1234zq": a password cut at ? or #
- credQuery = "qxabcxq": the query or fragment part, and a token
They are used in the broker URLs, the error texts and secretParts118.
cmd/ingestor/mqtt_credentials_r3_118_test.go: these tests share secretParts118, so the token= value and the log check (which used to search for "abc") now use credQuery.
The other _118 files (mqtt_log_credentials_118_test.go, mqtt_client_id_118_test.go) do not use these values. The exact-match case token=abc in TestBrokerForLogMarksRemovedUserinfo_118 is not a leak search and stays as it is.
Coverage is unchanged: masked user, password (cut at /, ?, #), query, fragment and token.

Tests [T]

cd cmd/ingestor && TMPDIR=<tmpfs> go test -race -count=1 -timeout 20m ./...: ok (705 s).
go test -race -count=200 -run 'TestStatsFileHasNoCredentials_118$': ok (60 s).
go vet ./...: ok.
gofmt -l: both touched files are clean. The 11 other files it lists were already unformatted on master; I checked each one against origin/master.

Number independence [T]

A temporary test (not committed) drove the real stats-file path (prepareMQTTSource, then StartStatsFileWriter) with 1234 disconnects, so the file contained "disconnectCount":1234.

With the old secretParts118: stats file leaks "1234", the test(ingestor): TestStatsFileHasNoCredentials_118 flakes when a stats number contains '1234' #250 symptom.
With the new constants: pass.

Mutant [T]

With errForLog returning err.Error(), 4 tests are red:

TestErrForLogMasksKnownSecrets_118
TestDisconnectErrorMasksKnownSecrets_118
TestSourceStatusHoldsNoCredentials_118
TestStatsFileHasNoCredentials_118 (leaks dev-user, s3cret-pass, tok3n)

After the file was restored, all pass again.

CI per job [K] (run 37305217906)

Go Build & Test: pass (both attempts).
Playwright E2E Tests: fail (both attempts). The only failure is test-issue-1122-details-row-clamp-e2e.js "advert links in Details stay visible and clickable" at 1200/900 px, reached 13m49s and 13m57s after freshen. That is the test: Details-clamp E2E 'advert links in Details stay visible and clickable' is time-dependent and flaky #244 flake, fixed by PR test(packets): make the Details advert-link E2E independent of fixture age (#244) #252. This PR does not touch the frontend or the E2E tests.
Release Artifacts, Build & Publish Docker Image, Deploy Staging and Publish Badges: skipped.

Remaining

E2E goes green once test(packets): make the Details advert-link E2E independent of fixture age (#244) #252 is on master and this branch is re-run against it. Not done here: no merge, and no rebase.

dborup-agent · 2026-10-05T13:11:10Z

Review — CS-pve-agent1 PR#253 cred-test — head `4e8f314`

Dom: APPROVE med nits

Read-only review of head 4e8f31474bb5, using the head tree and the merged tree (git merge-tree --write-tree origin/master with master d05b0db5), each extracted with git archive. Nothing was pushed or changed.

Evidence tags: [T] tested or run by me, [A] assumption or approximation, [K] known from code, CI logs or GitHub metadata.

Findings

#	Severity	Finding	Evidence
1	Nit (optional)	The old passwords `2024` and `1234` were port-shaped, so `url.Parse("tcp://dev-user:2024/s3cret-pass@host:1883")` succeeded and returned host `dev-user:2024`. That is the silent misparse #118 is about. With `pw2024pw` and `zq1234zq`, `url.Parse` now fails with `invalid port`. Production masking (`brokerurl`) never calls `url.Parse`, so `brokerForLog` loses no coverage (see the mutants below). One shape is gone, though. A mutant where `mqttClientID` calls `url.Parse` on the raw broker is still red, but now because the regex expects `corescope-host-…` and the ID is `corescope-398177e4`, not because the user name leaks (`corescope-dev-user-…` before). If you want to keep the shape, add one exact-match case with a digit-only, port-like password in `TestBrokerForLogAmbiguousPassword_118` or the client-ID test. Its value would not need to go into `secretParts118`.	[T]
2	Info (positive)	`secretParts118` is shared, so the change also removes the same flake risk from four other searches over text that contains numbers. They are the status-snapshot JSON (L120), the per-line log check with timestamps (L175), the "stats data" JSON (L184) and the `errForLog` output (r3 L58).	[K]

No blocking issues.

1. Is coverage kept? Yes [T]/[K]

[K] Each part is still in the brokers, the error texts and secretParts118:
- masked user dev-user;
- password cut at /: pw2024pw;
- password cut at ? and at #: zq1234zq;
- query and fragment: qxabcxq;
- token: tok3n, plus token= + credQuery in the r3 tests;
- bad escape: p%zz.
The only short value left, token=abc in TestBrokerForLogMarksRemovedUserinfo_118, is an exact-match check, not a leak search.

[T] I applied the same brokerurl mutants to master's tests and to the PR's tests. The failing _118 tests are identical in every case:

Mutant	Old tests red	New tests red
`errForLog` returns `err.Error()`	4 (ErrForLogMasksKnownSecrets, DisconnectErrorMasksKnownSecrets, SourceStatusHoldsNoCredentials, StatsFileHasNoCredentials)	the same 4
`Mask` splits like RFC 3986 (`split(s, true)`), so the `/` cut leaks	4 (AmbiguousPassword, MarksRemovedUserinfo, ClientIDBase, SourceStatus)	the same 4
`Secrets` drops query and fragment values	2 (ErrForLog…, DisconnectError…)	the same 2
`Mask` keeps the query	5 (AmbiguousPassword, MarksRemovedUserinfo, DisconnectError, SourceTagHasNoCredentials, SourceStatus)	the same 5
`mqttClientID` uses `url.Parse` on the raw broker	1 (ClientIDBase, `corescope-dev-user-…`)	1 (ClientIDBase, `corescope-398177e4`; see finding 1)

[T] With the errForLog mutant, the new TestStatsFileHasNoCredentials_118 reports leaks of dev-user, s3cret-pass and tok3n. The r3 tests report qxabcxq, so credQuery is exercised.

2. Independent of numbers? Yes [T]

I wrote a temporary test that was not committed and is now removed. It drove the real path: prepareMQTTSource, then 1234 × MarkDisconnect, then StartStatsFileWriter. It waited until the file contained "disconnectCount":1234 and then ran assertNoSecretParts118 on it.

With master's secretParts118: FAIL, stats file leaks "1234": {"sampledAt":…. This is the test(ingestor): TestStatsFileHasNoCredentials_118 flakes when a stats number contains '1234' #250 symptom.
With the PR's list: ok.

3. Mutant `errForLog` → `err.Error()` [T]

4 _118 tests are red, as listed above, the same on old and new. After restoring the file, all are green.

4. Repetition and full suite [T]

go test -race -count=50 -run 'TestStatsFileHasNoCredentials_118$': ok (19.8 s).
go test -race -count=10 -run '_118': ok.
cd cmd/ingestor && TMPDIR=<tmpfs> go test -race -count=1 -timeout 30m ./... on the merged tree: ok (836.8 s).
go vet ./...: ok. gofmt -l on both touched files: clean.
internal/brokerurl go test: ok.

CI [K]

In run 37305217906, Go Build & Test passes on both attempts.
Playwright E2E fails, but its only ✗ lines are [desktop-1200] and [tablet-900] advert links in Details stay visible and clickable … "R5-D4 300D Rak", reached 13m49s and 13m57s after freshen. That is the test: Details-clamp E2E 'advert links in Details stay visible and clickable' is time-dependent and flaky #244 flake that test(packets): make the Details advert-link E2E independent of fixture age (#244) #252 fixes. This PR touches neither the frontend nor the E2E tests. It needs an E2E re-run once test(packets): make the Details advert-link E2E independent of fixture age (#244) #252 is on master.

Other checks

[K] Only cmd/ingestor/mqtt_credentials_r3_118_test.go (+5/−5) and cmd/ingestor/mqtt_status_credentials_118_test.go (+18/−8) changed. No production code. The merged tree differs from master in those two files only.
[K] Fork guards (github.repository == 'Kpa-clawbot/CoreScope'): 9 in deploy.yml and 1 in release-fast-path.yml, the same on head, merged tree and master.
[K] No closing keywords (closingIssuesReferences is empty; the body says "Relates to test(ingestor): TestStatsFileHasNoCredentials_118 flakes when a stats number contains '1234' #250").
[K] Commit author and committer: dborup <kontakt@meshview.dk>. One commit on top of the merge base.

Not verified

I did not repeat -count=200; I ran -count=50 with -race.
I did not run cmd/server tests. This PR touches only ingestor tests.
I did not re-run this branch's E2E against a master that includes test(packets): make the Details advert-link E2E independent of fixture age (#244) #252.

dborup closed this Oct 5, 2026

dborup reopened this Oct 5, 2026

dborup marked this pull request as ready for review October 5, 2026 14:04

dborup merged commit 0572e7f into master Oct 5, 2026
17 of 19 checks passed

dborup mentioned this pull request Oct 5, 2026

test(ingestor): TestStatsFileHasNoCredentials_118 flakes when a stats number contains '1234' #250

Closed

2 tasks

Navigation Menu

Repository navigation

test(ingestor): use non-numeric secrets in the #118 stats-file leak test (#250) - #253

test(ingestor): use non-numeric secrets in the #118 stats-file leak test (#250)#253
dborup merged 1 commit into
masterfrom
codex/issue-250-stats-cred-test

dborup-agent commented Oct 5, 2026

Uh oh!

dborup-agent commented Oct 5, 2026

Uh oh!

dborup-agent commented Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

dborup-agent commented Oct 5, 2026

Problem

Change (test code only)

Evidence (local)

Uh oh!

dborup-agent commented Oct 5, 2026

Rapport — CS-pve-agent2 PR#253 #250 — head 4e8f314

Change (test code only)

Tests [T]

Number independence [T]

Mutant [T]

CI per job [K] (run 37305217906)

Remaining

Uh oh!

dborup-agent commented Oct 5, 2026

Review — CS-pve-agent1 PR#253 cred-test — head 4e8f314

Findings

1. Is coverage kept? Yes [T]/[K]

2. Independent of numbers? Yes [T]

3. Mutant errForLog → err.Error() [T]

4. Repetition and full suite [T]

CI [K]

Other checks

Not verified

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Rapport — CS-pve-agent2 PR#253 #250 — head `4e8f314`

Review — CS-pve-agent1 PR#253 cred-test — head `4e8f314`

3. Mutant `errForLog` → `err.Error()` [T]