fix(telemetry): scrub existing rows, and stop --author= being eaten (#3925)

2026-09-11 15:43:31 -04:00
parent bfa2d419f9
commit 18e3cf9f2f
3 changed files with 115 additions and 1 deletions
@@ -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."""