From 18e3cf9f2f4ed28d4df2c8a119baf3930ed7d4b5 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 11 Sep 2026 15:43:31 -0400 Subject: [PATCH] fix(telemetry): scrub existing rows, and stop --author= being eaten (#3925) --- .../0099_scrub_secrets_from_retrieval_logs.py | 104 ++++++++++++++++++ src/scribe/services/retrieval_telemetry.py | 5 +- tests/test_retrieval_query_scrubbing.py | 7 ++ 3 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 alembic/versions/0099_scrub_secrets_from_retrieval_logs.py diff --git a/alembic/versions/0099_scrub_secrets_from_retrieval_logs.py b/alembic/versions/0099_scrub_secrets_from_retrieval_logs.py new file mode 100644 index 0000000..c643d21 --- /dev/null +++ b/alembic/versions/0099_scrub_secrets_from_retrieval_logs.py @@ -0,0 +1,104 @@ +"""scrub credential-shaped spans out of retrieval_logs.query + +Revision ID: 0099 +Revises: 0098 +Create Date: 2026-09-11 + +`pre_tool_rule` retrieves against the RAW COMMAND TEXT and `write_path_rule` +against the code being written, so whatever was on the command line or in the +buffer is what `record_retrieval` wrote into `retrieval_logs.query`. A command +that exported a token stored the token (#3925). + +`services/retrieval_telemetry.scrub_secrets` closes that going forward — the +value never reaches the column. It cannot reach BACKWARDS, and this does: it +rewrites the rows already written. + +REDACTED IN PLACE, NOT DELETED. The rest of the row — score, threshold, +result count, duration, the near-miss record id — is legitimate evidence, and +it is what a threshold is tuned from. Deleting the row would throw that away +to remove a secret that lives in one column, so the column is what gets +rewritten. Rows with no credential in them are not touched at all. + +THE PATTERNS ARE INLINED RATHER THAN IMPORTED, deliberately, against the DRY +instinct. A migration is a frozen record of a change that already happened on +every install that ran it; importing the live patterns would mean this +migration quietly does something different next year than it did when it ran, +and two installs at the same revision would no longer be in the same state. +The Python twin in `services/retrieval_telemetry.py` is free to grow — this is +what ran here, once. The one thing that must not drift is coverage, and the +guard for that is `test_retrieval_query_scrubbing.py`, which tests the live +function rather than this copy. + +POSIX regex, not Python's. Postgres ARE supports the non-greedy `*?` the PEM +pattern needs, and `\\s`/`\\S`, so the shapes port directly. The `'gi'` flags +are global + case-insensitive, matching `re.sub` with `(?i)`. + +NO BARE `auth` IN THE ASSIGNED PATTERN. It matches `--author=`, so a commit +naming an address would have had the address redacted — evidence eaten for a +word that only looks credential-shaped. `AUTH_TOKEN` is still caught, by +`token`. + +Downgrade is a no-op, and honestly so: the original text is gone and a +migration cannot invent it back. Saying that plainly is better than a +downgrade that appears to restore something and does not. +""" +from alembic import op + +revision = "0099" +down_revision = "0098" +branch_labels = None +depends_on = None + +# Vendor-prefixed credentials — the prefix IS the tell, so no entropy guessing. +_TOKEN = ( + r"(fmcp_|flt_|ghp_|gho_|ghs_|ghu_|github_pat_|glpat-|xox[abprs]-" + r"|sk-[A-Za-z0-9]*-?|AKIA|ASIA)[A-Za-z0-9_\-]{12,}" +) +# A value handed to a secret-NAMED variable, in shell, env, YAML, JSON or a +# query string. The NAME identifies it, so the value can be anything. +_ASSIGNED = ( + r"([A-Za-z0-9_]*(token|secret|password|passwd|api[_-]?key|access[_-]?key)" + r"[A-Za-z0-9_]*)(\s*[:=]\s*[\"']?)([^\s\"'&]{8,})" +) +_AUTH_HEADER = r"(authorization\s*:\s*(bearer|basic|token)\s+)(\S+)" +_PEM = ( + r"-----BEGIN [A-Z ]*PRIVATE KEY-----(.|\n)*?-----END [A-Z ]*PRIVATE KEY-----" +) + + + +def _lit(pattern: str) -> str: + """A regex as a SQL string literal. + + A single quote inside a single-quoted SQL literal has to be DOUBLED, and + the assigned-value pattern contains two of them (it allows an optional + quote around the value). Left unescaped they close the literal early and + the migration dies on a syntax error — which is the whole reason this + helper exists rather than the patterns being pasted in inline. + """ + return pattern.replace("'", "''") + + +_SCRUB_SQL = f""" +UPDATE retrieval_logs +SET query = regexp_replace( + regexp_replace( + regexp_replace( + regexp_replace(query, '{_lit(_TOKEN)}', '[redacted:token]', 'gi'), + '{_lit(_ASSIGNED)}', '\\1\\3[redacted:assigned]', 'gi'), + '{_lit(_AUTH_HEADER)}', '\\1[redacted:auth-header]', 'gi'), + '{_lit(_PEM)}', '[redacted:private-key]', 'gi') +WHERE query IS NOT NULL + AND (query ~* '{_lit(_TOKEN)}' + OR query ~* '{_lit(_ASSIGNED)}' + OR query ~* '{_lit(_AUTH_HEADER)}' + OR query ~* '{_lit(_PEM)}') +""" + + +def upgrade() -> None: + op.execute(_SCRUB_SQL) + + +def downgrade() -> None: + """Deliberately empty — the original text no longer exists to restore.""" diff --git a/src/scribe/services/retrieval_telemetry.py b/src/scribe/services/retrieval_telemetry.py index d15c892..f942329 100644 --- a/src/scribe/services/retrieval_telemetry.py +++ b/src/scribe/services/retrieval_telemetry.py @@ -86,7 +86,10 @@ _SECRET_PATTERNS: tuple[tuple[str, "re.Pattern[str]"], ...] = ( # be anything. ("assigned", re.compile( r"(?i)\b([A-Za-z0-9_]*" - r"(?:token|secret|password|passwd|api[_-]?key|access[_-]?key|auth)" + # NO BARE "auth" HERE. It matched `--author=`, so a commit naming an + # address redacted the address — evidence eaten for a word that only + # LOOKS credential-shaped. `AUTH_TOKEN` is still caught, by `token`. + r"(?:token|secret|password|passwd|api[_-]?key|access[_-]?key)" r"[A-Za-z0-9_]*)" r"(\s*[:=]\s*[\"']?)" r"([^\s\"'&]{8,})" diff --git a/tests/test_retrieval_query_scrubbing.py b/tests/test_retrieval_query_scrubbing.py index 17c0807..c4a13ed 100644 --- a/tests/test_retrieval_query_scrubbing.py +++ b/tests/test_retrieval_query_scrubbing.py @@ -71,6 +71,13 @@ _EVIDENCE = [ # The word "token" in ordinary prose is not a token. "explain how the token bucket rate limiter works", "wc -l src/*.py && date", + # `--author=` contains "auth". A bare `auth` keyword in the assigned + # pattern redacted the address here, which is the evidence-eating failure + # this block exists to catch — and it shipped for one commit because the + # set did not contain a case with it. `AUTH_TOKEN=` is still caught, via + # `token`. + "git commit --author=bvandeusen@example.com -m 'x'", + "git log --author=\"Bryan Van Deusen\" --oneline", ]