Draft: feat(scrub): redact secrets embedded in strings (URL credentials, quoted inline assignments)

Summary

The ingest scrubber's key denylist redacts a field whose name looks sensitive. It does nothing when the secret is inside a string — which is how secrets usually reach an error tracker, because nobody chose to send them. An object gets stringified into an exception message or a stack-frame local and carries its constructor arguments along:

vars: { "creds": "Credentials(username='alice', password='hunter2')" }

Today that passes through untouched even with scrubbing enabled: the key creds matched nothing, and no value pattern looks for secrets in free text (only Luhn-valid card numbers, PEM blocks, and opt-in emails). Same for a connection string with an inline password, a common shape in connect-failure exceptions.

This adds two value patterns for that, plus two smaller recall fixes, and pays for part of them with hot-path work that was not buying anything.

Server-side scrubbing stays a backstop, not the primary control — the right answer is still not to send the data. This is for operators who don't control the SDKs sending to them.

Rust-only, deliberately. apps/event_ingest/pii_scrubber.py is left alone as the Python ingest path is retired. Rust-only behavior uses a separate assert_event_scrub helper so the parity suite keeps meaning exactly "both engines agree" rather than silently absorbing the divergence.

Commits

1. creds token + errors section — two recall gaps. errors is client-supplied, list[Any], and persisted into IssueEvent.data.
2. Perf refactor, no behavior change — CARD_RE ran unconditionally over every string; each pattern now has a byte-level precondition that is a strict superset of what its regex can match. Separately, replace_all returns Cow::Borrowed on no-match and the old code called .into_owned() on it regardless — one needless allocation + copy per string per active pattern on the common path.
3. The two new patterns — scrub_url_credentials and scrub_inline_assignments, each behind its own flag.
4. Fixes from independent review — see below.

What commit 4 fixes (please read before the rest)

Three independent fresh-context reviewers went at commits 1–3. They found two redaction bugs and an invalid benchmark. All are fixed in commit 4, with regression tests; I'm calling them out rather than burying them because two of them invalidate claims commits 1–3 made.

The URL pattern escaped the URL and destroyed data. The password group excluded only @, whitespace and /, so a host:port URL with no path followed anywhere later by an @ consumed everything between and replaced it:

{"url":"http://host:8080","contact":"a@b.com"}  →  {"url":"http://host:[Filtered]@b.com"}

Serialized JSON and logfmt are the common carriers, so this was the normal case. Scrubbing precedes persistence, so the loss was unrecoverable, and it was a regression — on main those strings pass through untouched. My tests missed it because every URL in them had a path and the / terminated the match.

A benign quoted wrapper hid every secret inside it. replace_all resumes past the whole match, and a quoted value body runs to its closing quote, so a non-sensitive key swallowed any nested assignment:

Error executing query: "UPDATE users SET password='hunter2'"   → unredacted
{'body': "password='hunter2'"}                                  → unredacted

Both are exactly the shapes this mechanism exists for. Non-sensitive bodies are now rescanned, depth-capped.

String-headed lists lost their tail. The [[key, value], …] fast path handled element [1] and skipped the rest. Now every element past the key is handled — a deliberate divergence from pii_scrubber.py.

What it catches, and what it doesn't

Requiring the value to be quoted is the false-positive defense: reprs, dict dumps and embedded JSON quote their strings; prose does not.

Input	Result
Credentials(username='alice', password='hunter2')	password='[Filtered]', username kept
{'password': 'hunter2', 'host': 'pg'}	redacted
{"api_key": "sk-live-123", "page": "2"}	redacted
"UPDATE users SET password='hunter2'" (nested in a benign value)	redacted
postgres://app_user:p4ssw0rd@db:5432/app	app_user:[Filtered]@db
redis://:s3cret@cache:6379/0 (empty user)	redacted
password='he\'s in' (escaped quote)	redacted whole
auth=failed, session expired, retry: 3	untouched
{"url":"http://host:8080","contact":"a@b.com"}	untouched
Credentials('alice', 'hunter2') — positional	miss, no key to match on
password=hunter2 — unquoted	miss by design → aggressive_key_match
postgres://u:aB3/xY9@h — / in password	miss, admitting / re-opens the escape
https://<token>:x-oauth-basic@ — secret in user position	miss, constant password redacted instead

Every miss has a test, so they stay documented rather than becoming folklore.

Performance — corrected

The benchmark in commit 2 could not exercise ASSIGN_RE at all. No string in the fixture contained a quote character, so the guard rejected every one and the pattern never ran. The 105.8 → 81.8 → 92.5 table in commit 3 measured the guard, not the mechanism, and its conclusion that both patterns fit inside the freed headroom is false for inline assignments.

The fixture now carries repr-shaped frame locals and an embedded-JSON extra value, and the arms are flag-isolated so each pattern's marginal cost is reproducible from the repo:

key denylist only            58.1 us/event
+ cards/PEM (pre-series)     67.9
+ url creds                  72.5   (+7%)
+ inline assign (default)   124.1   (+83%)
+ emails                    129.3
aggressive key match        227.6

So: URL credentials are nearly free. Inline assignments roughly double the scrub. Absolute figures move a lot between machine states on this laptop — trust the ratios within a run, not the numbers.

cd crates/gt-ingest && cargo test --release scrub::tests::bench -- --ignored --nocapture

Open decisions

1. Should scrub_inline_assignments default on? It currently does. I recommended that before the +83% number existed, and the justification I gave ("lands inside the freed headroom") was wrong. Against it: the cost, plus it is the one mechanism that can remove text an operator wanted — source lines in context_line are also key = "value" shaped, and redacting inside exception.value changes the derived issue title and therefore the grouping hash. For it: it is the only thing here that fixes the repr-shaped leak, and it only fires when someone has already set enabled: true. My updated recommendation is to default it off; flipping is one word plus test-config churn.

2. The depth bound fails open. Past MAX_SCRUB_DEPTH (48) the subtree is persisted unscrubbed; serde_json accepts to 128, so 49–128 is reachable and extra is dict[str, Any]. For a redaction control, "too deep to inspect" arguably should mean "redact". That is data loss on deep-but-benign payloads and a divergence from long-standing behavior, so I have not changed it.

3. CPU amplification. A 5 MB payload of a:'' costs ~159 ms, and gzips ~510:1, so a ~10 KB POST buys that. Not superlinear — verified 2.00×±0.05 per doubling, so no ReDoS — but scrub_event runs inline with no yield point on a runtime worker shared across tenants. Mitigation touches pipeline.rs, outside this change.

Known-good follow-ups, measured but not included

Left out to keep commit 4 to the correctness bugs:

• is_sensitive_key is ~67% of total scrub time and allocates 3.7× per key; 82% of real keys are plain lowercase ASCII needing no normalization. A borrow-only fast path measured −32% on that function.
• The four guards are four separate passes over the same bytes and are ~76% of scrub_string's cost; one fused pass measured −60%.
• Implementing regex::Replacer::replace_append instead of closures returning String removes an allocation per non-sensitive match.

Together these would likely absorb a good part of the inline-assignment cost, which is relevant to decision 1.

Still outstanding

• No UI. Project.scrub_config is editable via Django admin as raw JSON and nothing else.
• No org-level tier, unlike scrub_ip_addresses.
• Backend GLITCHTIP_RUST_INGEST still defaults to False, so none of this is live until that flips.

Testing

./scripts/check.sh passes (fmt + clippy + tests across the extension and all four standalone core crates). 46 scrub tests, 14 new. The two *_matches_python parity tests pass untouched; one pair-shape case moved to the Rust-only suite where the divergence is deliberate.

Not exercised end-to-end through a live ingest path — unit-level only.

AI disclosure: Claude Opus 5 (Claude Code) — investigated the gap, wrote the patch, tests and benchmark, ran the adversarial self-review that caught the bugs in commit 4, and drafted this description. Reviewed and directed by me. The design calls (Rust-only, quoted-values-only, no size cap, per-flag split) were discussed and agreed before implementation; the open decisions above are genuinely open. Not yet human-tested against a live install — hence draft.