diff --git a/src/scribe/services/retrieval_telemetry.py b/src/scribe/services/retrieval_telemetry.py index bffdfc1..d15c892 100644 --- a/src/scribe/services/retrieval_telemetry.py +++ b/src/scribe/services/retrieval_telemetry.py @@ -17,6 +17,7 @@ from __future__ import annotations import asyncio import logging +import re from typing import Any from datetime import datetime, timedelta, timezone @@ -44,6 +45,81 @@ _pending: set[asyncio.Task] = set() _reported = False + +# ── secrets never reach the query column (#3925) ─────────────────────── +# +# `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 gets logged. A command that exports a token therefore stored +# the token — and worse than stored it: `near_miss_samples` is the readout the +# threshold docs tell you to open before moving a bar, so the value came back +# out into an agent's context on the next tuning pass. That is how this was +# found. +# +# SCRUBBED ON WRITE, NOT ON READ. A read-side filter leaves the secret in the +# table, where a backup, a debug query or a future readout still reaches it. +# The value must never land. +# +# REDACTED VISIBLY, AND THIS IS THE PART THAT KEEPS THE READOUT HONEST. The +# whole worth of a near-miss sample is reading the query that was actually +# refused; a scrubber that silently deleted spans would turn the one instrument +# for tuning a bar into unreadable stubs — the #2663 shape, where a surface +# looks fine and has quietly stopped saying anything. A `[redacted:]` +# marker keeps the sentence readable, keeps its shape and length roughly +# intact for the ranker's reader, and says plainly that something was removed. +# +# DELIBERATELY CONSERVATIVE. These patterns match things that are secrets by +# CONSTRUCTION — a vendor-prefixed credential, a value assigned to a +# secret-named variable, an auth header, a PEM header. Anything cleverer +# (entropy heuristics, long-opaque-string detection) starts eating real +# queries, and a query is evidence. Missing an exotic secret costs one +# redaction nobody made; eating a query costs the ability to tune the bar. +_SECRET_PATTERNS: tuple[tuple[str, "re.Pattern[str]"], ...] = ( + # Vendor-prefixed credentials. The prefix IS the tell, so no entropy + # guessing is needed — `fmcp_` is Scribe's own API key format. + ("token", re.compile( + r"\b(?:fmcp_|flt_|ghp_|gho_|ghs_|ghu_|github_pat_|glpat-|gitlab-ci-token:" + r"|xox[abprs]-|sk-[A-Za-z0-9]*-?|AKIA|ASIA)[A-Za-z0-9_\-]{12,}" + )), + # A value handed to a secret-NAMED variable, in shell, env files, YAML, + # JSON or a query string. The name is what identifies it, so the value can + # be anything. + ("assigned", re.compile( + r"(?i)\b([A-Za-z0-9_]*" + r"(?:token|secret|password|passwd|api[_-]?key|access[_-]?key|auth)" + r"[A-Za-z0-9_]*)" + r"(\s*[:=]\s*[\"']?)" + r"([^\s\"'&]{8,})" + )), + ("auth-header", re.compile( + r"(?i)(authorization\s*:\s*(?:bearer|basic|token)\s+)(\S+)" + )), + ("private-key", re.compile( + r"-----BEGIN [A-Z ]*PRIVATE KEY-----[\s\S]*?-----END [A-Z ]*PRIVATE KEY-----" + )), +) + + +def scrub_secrets(text: str | None) -> str | None: + """Redact credential-shaped spans from a query before it is stored. + + Pure and synchronous, so it is unit-testable and safe to run inline on the + write path. Returns the input unchanged when nothing matches, which is the + overwhelmingly common case and the one the patterns are tuned to protect. + """ + if not text: + return text + for kind, pattern in _SECRET_PATTERNS: + if kind == "assigned": + text = pattern.sub( + lambda m: f"{m.group(1)}{m.group(2)}[redacted:{kind}]", text) + elif kind == "auth-header": + text = pattern.sub(lambda m: f"{m.group(1)}[redacted:{kind}]", text) + else: + text = pattern.sub(f"[redacted:{kind}]", text) + return text + + def _build_payload( *, user_id: int | None, @@ -85,7 +161,10 @@ def _build_payload( return { "user_id": user_id, "source": source, - "query": query, + # Scrubbed HERE rather than at each caller: this is the only path to + # the column, and a per-caller scrub is three places for one of them + # to be forgotten by whoever adds the fourth arm. + "query": scrub_secrets(query), "threshold": threshold, "limit_n": limit, "project_id": project_id, diff --git a/tests/test_retrieval_query_scrubbing.py b/tests/test_retrieval_query_scrubbing.py new file mode 100644 index 0000000..17c0807 --- /dev/null +++ b/tests/test_retrieval_query_scrubbing.py @@ -0,0 +1,128 @@ +"""A logged query never carries a credential (#3925). + +WHY THIS EXISTS + +`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` stores in `retrieval_logs.query`. A command +that exported a token therefore stored the token. + +And storing it was not the worst of it. `near_miss_samples` is the readout the +threshold documentation tells you to open before moving a bar, so the value +came back OUT into an agent's context on the next tuning pass — which is +exactly how this was found, during #3853's threshold spike. + +WHAT THIS PINS, IN BOTH DIRECTIONS, AND WHY THE SECOND HALF IS THE HARD ONE + +A scrubber has two ways to fail and only one of them is obvious. + + 1. It misses a secret. Caught by the redaction cases below. + 2. It eats the EVIDENCE. This is the failure that would do more damage, + because it is silent: the whole worth of a near-miss sample is reading the + query that was actually refused, and a scrubber that chewed up ordinary + commands would turn the one instrument for tuning a bar into unreadable + stubs while still looking like it worked. That is the #2663 shape — a + surface that reads fine and has quietly stopped saying anything. + +So the second block is not padding. Its cases are REAL queries taken from this +install's `near_miss_samples` during #3853, and they must survive byte for +byte. If a future pattern is added and one of them changes, the pattern is too +greedy — tighten it rather than editing the expectation. + +The secret cases use FABRICATED values in the real formats. Nothing here is or +was a live credential. +""" +import pytest + +from scribe.services.retrieval_telemetry import scrub_secrets + +# Fabricated, in the shapes that actually occur. The first is the shape that +# was found stored: a shell assignment of a vendor-prefixed token. +_SECRETS = [ + ("vendor-prefixed token in a shell assignment", + "TOK=flt_AAAABBBBCCCCDDDDEEEEFFFF\npython3 - <<'PY'", + "flt_AAAABBBBCCCCDDDDEEEEFFFF"), + ("a Scribe fmcp_ key in an auth header", + "curl -H 'Authorization: Bearer fmcp_ZZZZYYYYXXXXWWWWVVVV' https://x", + "fmcp_ZZZZYYYYXXXXWWWWVVVV"), + ("a forge token in an export", + "export GITHUB_TOKEN=ghp_1234567890abcdefghijABCDEF", + "ghp_1234567890abcdefghijABCDEF"), + ("a value assigned to a secret-named variable", + 'REGISTRY_PASSWORD="hunter2-correct-horse"', + "hunter2-correct-horse"), + ("an api_key in a query string", + "curl 'https://api.example/v1/things?api_key=abcdef1234567890'", + "abcdef1234567890"), + ("a private key block", + "-----BEGIN RSA PRIVATE KEY-----\nMIIEowIBAAKC\n-----END RSA PRIVATE KEY-----", + "MIIEowIBAAKC"), +] + +# Real queries, from this install's near-miss samples during #3853. +_EVIDENCE = [ + "git push origin dev", + 'git pull --rebase origin dev 2>&1 | tail -3; echo "=== HEAD ==="; git log --oneline -2', + "python3 - <<'PY'\nimport pathlib\np = pathlib.Path(\"web/src/routes/admin/tuning/tuning.test.ts\")", + "docker compose up -d", + 'package library\n\nimport (\n\t"context"\n\t"fmt"\n)', + "import {\n fetchTransfers,\n retryTransfer,\n} from './api'", + 'grep -rn "useState" src/components/ | head -20', + # The word "token" in ordinary prose is not a token. + "explain how the token bucket rate limiter works", + "wc -l src/*.py && date", +] + + +@pytest.mark.parametrize(("label", "text", "secret"), _SECRETS, + ids=[c[0] for c in _SECRETS]) +def test_a_credential_never_survives_into_the_query_column(label, text, secret): + """The value goes; something visible stays in its place.""" + out = scrub_secrets(text) + assert secret not in out, ( + f"{label}: the credential is still in the text that would be stored" + ) + assert "[redacted" in out, ( + f"{label}: the span was removed without saying so. A silent deletion " + "leaves a reader unable to tell a scrubbed query from a short one, " + "which is the readout lying about itself rather than protecting you." + ) + + +@pytest.mark.parametrize("query", _EVIDENCE) +def test_an_ordinary_query_is_stored_exactly_as_it_was(query): + """Evidence survives byte for byte. + + These came out of real `near_miss_samples`. A threshold is tuned by reading + them, so a pattern greedy enough to touch one has destroyed the instrument + it was meant to make safe — tighten the pattern, never this expectation. + """ + assert scrub_secrets(query) == query + + +def test_empty_and_missing_queries_pass_through(): + """Some sources log no query at all; scrubbing must not invent one.""" + assert scrub_secrets(None) is None + assert scrub_secrets("") == "" + + +def test_the_write_path_scrubs_rather_than_the_read_path(): + """The payload built for storage carries the redacted text (#3925). + + Pinned on `_build_payload` because that is the single seam every source + reaches the column through. A per-caller scrub would be three places for + one of them to be forgotten by whoever adds the fourth arm — and the one + forgotten would be the one that stored a secret. + """ + from scribe.services.retrieval_telemetry import _build_payload + + payload = _build_payload( + user_id=1, source="pre_tool_rule", + query="export API_TOKEN=ghp_1234567890abcdefghijABCDEF && git push", + threshold=0.68, limit=5, project_id=0, is_task=None, + results=[], duration_ms=1.0, + ) + assert "ghp_1234567890abcdefghijABCDEF" not in payload["query"] + assert "[redacted" in payload["query"] + # The rest of the command survives, or the row stops being evidence. + assert "git push" in payload["query"]