effective_config guard blind spots, is_secret over-matching, and the JSON type change from redaction

[fcostaoliveira]

Three known limits in the effective_config guards and redactor added by #264. None is a live defect today; all are drift risk, and each is written down here rather than left implicit.

1. Guard 1 has two blind spots

no_production_code_reads_the_environment_outside_the_recorder is a substring scan. Two shapes evade it:

• Braced import. use std::env::{var}; produces no use std::env::var substring, and the call site is then a bare var("X") with no env::var( prefix. Invisible.
• env_var(&expr). Guard 2 counts non-literal env_var sites to pin the one legitimate dynamic caller, but excludes anything starting with & — so env_var(&name.to_uppercase()) is hidden from the dynamic count by the & exclusion and from the literal-name list by being non-literal. Invisible to both directions. The & exclusion has no current user and can probably just be dropped.

The real fix for both is to stop matching text: walk the token stream (or a syn parse) instead.

2. is_secret over-matches, and changes a JSON type

scrub_value replaces any leaf under a credential-named key with the string <redacted:set>, so a number becomes a string — session_pool_size: 8 → "<redacted:set>". A consumer that expects a number gets one only when the key happens not to match.

The marker list is contains over 13 substrings, so plausible-but-benign keys match: tokenizer (a real ES/OS analyzer key), password_policy, session_pool_size. And strip_userinfo's conservative fallback rewrites http://reporting.internal:8080/dashboards/team@ops → http://<redacted:set>@ops, destroying host, port and path.

Measured today: zero false positives across all 31 shipped configs and all 104 recorded knob names — every match is a genuine credential. The bias towards over-redaction is deliberate (a redacted knob costs provenance; a published one costs a rotation). But the type change deserves a decision: emit a typed placeholder, or keep the value's type and blank only strings.

3. Recording proves presence, not identity

effective_config::Recording is move-only and consumed by run_single_experiment, which makes a commented-out, conditional, or hoisted begin_experiment a compile error. It does not prove the token belongs to the experiment being run: handing run_single_experiment a token minted for a different (config, dataset) pair compiles and passes.

Closing that means carrying the config name and dataset in the token and checking it at the far end. The ordering half ("the recording begins before the engine is built") is covered by a source-scanning guard instead.

[fcostaoliveira]

Update after the fifth review pass — most of item 2 is now moot, and item 1 has grown.

Item 2 (is_secret over-matching) is largely superseded. #264 now rebuilds connection-like strings from an ALLOW-list (redact_connection_like) instead of block-listing secret names inside them, so the marker gaps this issue lists — AccountKey=, SharedAccessSignature=, X-Amz-Signature=, bearer=, percent-encoded %70assword=, Cyrillic-homoglyph pаssword= — no longer matter for values inside a connection string: unrecognised parameter names have their values dropped without anyone needing to have predicted them.

What remains from item 2 is narrower and still worth doing:

• is_secret still governs whole values by KEY name, so a credential under an unlisted key (X_SIGNING_KEY-style names not covered) is still published.
• The JSON type change is unchanged: a number under a credential-named key becomes the string <redacted:set>.
• New, from the allow-list: SAFE_PARAM_KEYS is a hand-maintained list with no bidirectional guard, unlike KNOWN_UNREAD / KNOWN_UNRECORDED / EXEMPT_CALLS. Adding a parameter there is a decision to publish its value, and nothing forces that decision to be revisited.

Item 1 (guard blind spots) should also record what has since been found and fixed, because the pattern is the point: strip_test_modules had seven distinct over-strip bugs found by hand across five review passes — #[cfg(test)] const, bodiless mod X;, //-commented closer, block-commented fake module, block comment after a bodiless mod, // inside a string literal, and /* inside a /// doc comment. It now has a table-driven test pinning all seven, but it is still a text scanner where a token-stream or syn parse belongs. The two blind spots this issue already names (use std::env::{var}, env_var(&…)) are unchanged.

Item 3 (Recording proves presence, not identity) is unchanged and still accurate.

[fcostaoliveira]

Re-check of every item against 832f613 (current #264 head) and against master 7899014

Two structural facts first, because they decide what this issue can and cannot become:

1. None of this exists on master. effective_config.rs, is_secret, SECRET_MARKERS, scrub_value, redact_connection_like and both guards are added by #264 and by nothing else.

$ git ls-tree -r --name-only 7899014 | grep -i effective
$ git grep -l "SAFE_PARAM_KEYS\|is_secret" 7899014 -- .
$ git grep -l "SAFE_PARAM_KEYS\|is_secret" 832f613 -- .
832f613:src/bin/vector_db_benchmark/effective_config.rs

So this issue cannot be closed by a PR based on master — there is no code there to guard. Every live item here is a follow-up to #264 and has to land either in #264 or after it.

2. SAFE_PARAM_KEYS no longer exists. It was introduced in b5d778d, touched in badf8e5, and deleted in 1896e5c "digest every structured value; delete the parser entirely". grep SAFE_PARAM_KEYS on 832f613 returns nothing, and so does grep strip_userinfo.

---

Item-by-item

item (as filed / as amended)	verdict on 832f613
1a. use std::env::{var} braced-import blind spot	LIVE — proven below
1b. env_var(&…) invisible to both directions	LIVE — proven below
2a. SAFE_PARAM_KEYS has no bidirectional guard	CLOSED — the list is gone with the parser
2b. connection-string marker gaps (AccountKey=, %70assword=, homoglyphs)	CLOSED by redact_connection_like (already recorded in the fifth-pass comment)
2c. strip_userinfo destroying host/port/path	CLOSED — the function is deleted
2d. is_secret over-matching by key name	LIVE, still zero measured impact
2e. is_secret under-matching (credential under an unlisted key)	LIVE
2f. the JSON type change	LIVE, and now carries an in-code tracked in #289 note
3. Recording proves presence, not identity	unchanged — not re-examined here

Evidence

Baseline at 832f613, cargo test --lib --bins, six targets:
129 + 0 + 0 + 0 + 2 + 875 = 1006 passed; 0 failed.

1a. Added to engine/turbopuffer.rs (production, not exempt, not UNCOMPILED):

use std::env::{var};

pub fn blindspot_a() -> String {
    var("VDB_BLINDSPOT_A").unwrap_or_default()
}

A production read of the environment that never reaches the recorder. The guard runs and passes:

running 1 test
test effective_config::recorder_coverage_guard::no_production_code_reads_the_environment_outside_the_recorder ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 874 filtered out; finished in 0.41s

and the whole suite stays green: running 875 tests / test result: ok. 875 passed; 0 failed.

1b. Reverted 1a, then added:

pub fn blindspot_b(name: &str) -> String {
    crate::effective_config::env_var(&name.to_uppercase()).unwrap_or_default()
}

Invisible to the literal-name inventory (the argument is not a "-prefixed literal) and to the dynamic-site count (!rest.starts_with('&') excludes it). Guard 2 runs and passes:

running 1 test
test effective_config::recorder_coverage_guard::known_unrecorded_matches_the_env_var_call_sites_exactly ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 874 filtered out; finished in 0.77s

whole suite 875 passed; 0 failed.

2d/2e/2f. Temporary probe calling scrub_value and is_secret directly:

I289-TYPE {"session_pool_size":"<redacted:set>","port":6379}
I289-SECRET session_pool_size => true
I289-SECRET tokenizer => true
I289-SECRET password_policy => true
I289-SECRET authsource => true
I289-SECRET target_session_attrs => true
I289-SECRET X_SIGNING_KEY => true
I289-SECRET GOOGLE_APPLICATION_CREDENTIALS => true
I289-SECRET VDB_ACCOUNT_PAT => false
I289-SECRET REDIS_KEY_PREFIX => false

The type change is exactly as filed: 8 in, "<redacted:set>" out, with the sibling port keeping its number.

One correction to the record: authsource and target_session_attrs were removed from SAFE_PARAM_KEYS, but is_secret still returns true for both. Had the list survived, the natural assertion "no SAFE_PARAM_KEY may be is_secret" would still fail on those two names. The point is moot only because the list is gone.

X_SIGNING_KEY => true — the amended comment lists X_SIGNING_KEY-style names as the under-match residual, but that particular name matches SIGNING_KEY. The residual is real (VDB_ACCOUNT_PAT => false); this example is not.

All probes reverted; grep -rn "i289_probe\|blindspot_a\|blindspot_b" src/ → no matches.

Suggested rescope

Items 2a/2b/2c are done — worth striking from the body so the issue stops advertising work that is finished. What is left is:

1. use std::env::{var} and env_var(&…) (item 1) — the & exclusion still has no user and can be dropped outright; the braced import wants a token-stream walk, or at minimum probes for use std::env::{ and a bare var(/vars( after one.
2. The JSON type change (2f) — a decision, not a bug.
3. is_secret under-matching (2e) — unfixable by construction; worth stating as an accepted limit rather than a TODO.
4. Item 3, unchanged.

None of it can be based on master.

[fcostaoliveira]

Update from #264's side. Two of the four live items are fixed on the branch (aded00f), and I want the remaining scope to be accurate rather than inherited.

Fixed — both reproduced against 832f613 first

Braced import. use std::env::{var}; matched neither alias probe: there is no use std::env::var substring, and the call site is then a bare var("X") with no env:: prefix. Invisible end to end — adding it to a production, non-exempt file left the guard green.

The fix bans the import shape rather than trying to recognise the call: use std::env::{ and use std::env as now join the alias probes. A token-stream walk would be the thorough answer, but this is a line scan and the bare call is exactly what a line scan cannot see — so banning the only shape that produces it is the honest fix at this level. Verified CAUGHT, including use std::env as e.

env_var(&…). The !rest.starts_with('&') exclusion is deleted. Your diagnosis was exact: it hid the site from both inventories at once — non-literal so the name matching never saw it, &-prefixed so the dynamic count never saw it either. The exclusion had no remaining user (index_naming moved to env_flag several rounds ago), and the one legitimate dynamic site, opensearch::parse_env_secs, is still accounted for.

Remaining, and they are decisions rather than bugs

• JSON type change — a number under a credential-named key becomes the string <redacted:set>. Still true, still deliberate, still zero false positives across shipped configs.
• is_secret under-match — thank you for the correction: X_SIGNING_KEY does match the SIGNING_KEY marker, so it was a bad example. VDB_ACCOUNT_PAT is the real residual. Unchanged: no shape test can protect a punctuation-free token, and is_secret on the key is the sole defence for one.

Also correcting my own record

I claimed that removing authsource and target_session_attrs from SAFE_PARAM_KEYS made "no SAFE_PARAM_KEY may be is_secret" enforceable. is_secret still returns true for both, so the conflict was sidestepped rather than resolved — the guard passed because the names left the list, not because the two inventories were reconciled. It is moot now only because SAFE_PARAM_KEYS and its guard were both deleted when every structured value became a digest. #264's body has been corrected.

Once #264 merges, this issue reduces to the two decisions above plus the token-stream walk as a possible follow-up.