fix(telemetry): the redaction floor missed every env-var-shaped secret, and nothing could tell (backend#1908)

[LukasWodka]

Groundwork for backend#1908 (Class B), and a live gap found on the way. Two findings, one of them a real leak.

1. The check could not fail

tests/telemetry_collector_test.yaml carried it: carries all six of controller.py's secret patterns. It greps the rendered config for six substrings it holds itself; controller.py lives in client-runtime and is never read. So it agreed with itself and reported that as agreement with a file it has no access to.

Add a seventh redaction over there and every assertion stays green while the Collector ships the secret it does not know about. That is CLAUDE.md rule 1, and rule 9's corollary about testing a list against itself. Renamed to what it actually checks.

2. The floor had a hole — found the first time anything fed it a real string

The generic assignment pattern anchored its keyword with \b on both sides:

(?i)\b(password|passwd|pwd|secret|api[_-]?key|access[_-]?key|client[_-]?secret)\b(\s*[:=]\s*)[^\s"',;]+

_ is a word character, so a prefix or suffix removes the boundary and the whole assignment falls out. Measured:

line	before	after
password=s3cr3t-p4ss	redacted	redacted
AZURE_CLIENT_SECRET=…	leaks	redacted
MYSQL_PASSWORD=…	leaks	redacted
DJANGO_SECRET_KEY=…	leaks	redacted
AWS_SECRET_ACCESS_KEY=…	leaks	redacted
SECRET_KEY='x9y8z7'	leaks	redacted

It caught the bare form and missed every environment-variable spelling — which is the shape our logs actually print. backend#2069's Gap 1 deliberately names whichAZURE_* variable is unset when provisioning fails. The quoted value fell out separately: the value class excludes quotes, so it matched nothing after the separator.

Widened with [\w.-]* on both sides plus an optional opening quote. Over-redaction is the stated trade in controller.py's own comment; the guard below bounds it.

3. The guard is behavioural, not textual

scripts/tests/collector-redaction-floor.sh, added to DRIFT_GUARDS (now 15).

It never compares pattern text. It parses the replace_pattern regexes out of the rendered config — derived, so a pattern added or removed is seen — and applies them to specimens written down independently of the matcher. The question is the only one that matters: would this line leave the cluster with the secret still in it?

And it asks the reverse, which nothing asked before: does the diagnosis survive? Class B exists so an ingestion failure is diagnosable at full fidelity. A carelessly widened pattern that eats the traceback is not a win, and until now nothing would have noticed.

Three fail-closed paths: an unrenderable or unparseable config; zero patterns found; and a count floor, because specimens alone cannot see a removal that a broader pattern happens to mask.

Why this matters more than it looks

The Collector is Class A today — containers this chart owns. #1908 opens Class B: training and ingestion output, i.e. customer cell values and customer credentials. The redaction floor is the whole thing standing between that and central storage. A floor asserted by a check that cannot fail is not a floor.

Verification

helm unittest client -> 542 passed, 34 suites, 0 failures
make drift -> all 15 guards green
shellcheck -S warning -x -> clean bash -n -> ok

Mutation-proven three ways, anchor asserted applied each time and restored:

mutation	result
revert the pattern to its pre-fix form	the four env-var specimens redden, by name
delete a whole replace_pattern	count floor: 5 pattern(s)… floor is 6
replace one with a catch-all [a-z]+=[^ ]+	diagnosis direction: experiment=… -> '*** *** ***'

One correction against myself, for the record. The guard's first run reported eight leaking specimens. Seven were my bug, not the product's: I compiled the regexes without OTTL's string-literal unescaping, so \\s became a literal-backslash class that silently refused every specimen containing an s. Fixed, and the failure mode is written into the guard so the next person does not spend the cycle. Only the env-var finding survived scrutiny.

What this still cannot see, stated rather than implied

controller.py itself. CI checks out one repository, so a seventh redaction added in client-runtime is invisible here until someone adds its specimen. This PR therefore creates a divergence I am fixing in the same session:controller.py has the identical \b hole and needs the same widening, as a separate PR on client-runtime. Closing the drift properly needs one declaration both repos consume — filed separately.

This guard shrinks the gap from three copies, no checks to one cross-repo copy, with the behavioural floor pinned on this side.

---

Note

High Risk
Changes secret-redaction regexes on logs that will later include customer credentials (Class B). A too-narrow or too-wide pattern either leaks secrets or strips diagnosis.

Overview
Fixes a live leak in Collector log redaction: the generic assignment pattern used \\b on both sides of keywords, so env-var spellings (AZURE_CLIENT_SECRET=, MYSQL_PASSWORD=, DJANGO_SECRET_KEY=, AWS_SECRET_ACCESS_KEY=) and quoted values never matched. The pattern now allows a prefix and an optional opening quote, without a free suffix that would eat SharedAccessKeyName.

Adds scripts/tests/collector-redaction-floor.sh to DRIFT_GUARDS. It derives regexes from the rendered config and applies them to independent secret specimens plus diagnosis lines that must stay intact (count floor of 6, fail-closed). The helm-unittest case is renamed so it no longer claims agreement with controller.py. Chart bump to 1.9.64; the fixture path is allowlisted in .gitleaks.toml.

^{Reviewed by Cursor Bugbot for commit 4da2a3e. Bugbot is set up for automated code reviews on this repo. Configure here.}

[saqlainsyed007]

Verified against the regex, the test, and the gitleaks scope — this is a real security fix done carefully.

The leak was real. The generic assignment pattern anchored the keyword with \b on both sides, and since _ is a word char, every env-var spelling escaped redaction — AZURE_CLIENT_SECRET=, MYSQL_PASSWORD=, DJANGO_SECRET_KEY=, AWS_SECRET_ACCESS_KEY= — the exact shapes the logs print (backend#2069 names the unset AZURE_* var). The fix adds a leading [\w.-]* prefix and an optional opening quote, and — the part I most want to see — documents a rejected free-trailing-suffix fix that would have over-redacted SharedAccessKeyName (a non-secret key name kept for diagnosis), keeping the compound endings in the alternation instead. That's the right, precise call.

The floor test is derived, mutation-proof, fail-closed, and tests both directions. It parses the replace_pattern regexes out of the RENDERED config (never restates them), applies them to 14 independently-written secret specimens (rule 9 — not derived from the matcher, so they can't agree by construction), and separately asserts the DIAGNOSIS lines (epoch=, status:, Traceback) survive untouched so a widened pattern can't start eating the diagnosis. A MIN_PATTERNS count floor catches a removal another pattern masks; unrenderable config / zero patterns / an uncompilable pattern each fail closed. It even documents the two-level-escaping bug it hit (skipping the unescape made [^:/@\s] silently match and 8/10 specimens spuriously "leak") — exactly the "getting derivation wrong weakens the guard invisibly" awareness.

The vacuous test is correctly retired. The old "carries all six of controller.py's secret patterns" grepped six substrings it held itself against a file in another repo it never reads — agreeing with itself. Renamed to what it actually checks ("names all six redaction families in the rendered config"), with the behavioural floor moved to the new guard, and the residual cross-repo gap (controller.py) stated honestly, not hidden.

.gitleaks.toml is scoped right: path-anchored to the single fixture file, with the cost (a real secret pasted there later wouldn't be caught) weighed explicitly against the worse alternatives (suppressing the rule / baselining). Not a blanket disable. Chart version bumped (1.9.64), gate green.

CI green, Bugbot pass, no threads, added to DRIFT_GUARDS. This is the floor standing between Class B (customer credentials) and central storage, and it now actually holds. LGTM.