CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / integration (push) Successful in 49s
CI & Build / TypeScript typecheck (push) Successful in 55s
CI & Build / Python tests (push) Successful in 1m26s
CI & Build / Build & push image (push) Successful in 24s
Found post-deploy, by reading the telemetry it had just rewritten. The token it was written for IS gone — `TOK=[redacted:token]` where a credential used to be. But `<task-notification>` came back as `<ta[redacted:token]>`, across rows, because `sk-` matched INSIDE the word: `sk-` + `notification` is a vendor prefix followed by twelve word characters. TWO PORTING MISTAKES, COMPOUNDED. The live scrubber's pattern begins with `\b`; the migration's inlined copy had no boundary at all, dropped when I ported it to SQL. And `\b` would not have saved it either — in Postgres ARE `\b` is a BACKSPACE, not a word boundary. `\m` (start of word) is the spelling that means what Python's `\b` means. Two things that look interchangeable, are not, and fail in the same direction. The live scrubber was never affected, and the evidence says so cleanly: rows written after the deploy carry `<task-notification>` intact, while migration-rewritten ones are mangled. Only the frozen copy was wrong. THE DAMAGE HERE IS PERMANENT. The UPDATE overwrote the only copy of that text, so those rows cannot be restored. What this fixes is every OTHER install: 0099 has run exactly once, on one instance, and shipping a known evidence-destroying migration in the chain for everyone else would be the worse half of the mistake. The docstring records what it cost rather than tidying it away. The guard pins the property no reader can eyeball — `\m` present, `\b` absent, in both patterns — and is falsified against the shape that shipped. This is the third time this scrubber has eaten evidence it should not have (`--author=`, then `task-notification`), and the pattern is consistent: the redaction half is easy to verify and the SURVIVAL half only fails on inputs I did not think to include. The evidence set is where the work is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ
175 lines
8.0 KiB
Python
175 lines
8.0 KiB
Python
"""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",
|
|
# `--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",
|
|
]
|
|
|
|
|
|
@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"]
|
|
|
|
|
|
# ── the SQL twin has its own word-boundary spelling (#3925) ─────────────
|
|
#
|
|
# Migration 0099 carries an inlined copy of these patterns, deliberately: a
|
|
# migration is a frozen record of what already ran, and importing the live
|
|
# ones would mean it quietly did something different next year.
|
|
#
|
|
# Frozen is not the same as correct, and the first cut was neither. The
|
|
# boundary was dropped in the port, so `sk-` matched inside any word
|
|
# containing it — `<task-notification>` became `<ta[redacted:token]>` across
|
|
# thousands of rows on the one install that ran it. And writing `\b` would not
|
|
# have saved it: in Postgres ARE `\b` is a BACKSPACE, not a word boundary.
|
|
# `\m` (start of word) is the spelling that means what Python's `\b` means.
|
|
#
|
|
# So this pins the property no reader can eyeball, and it is a PRESENCE check
|
|
# on a token that must appear rather than an absence check on prose (#3352).
|
|
|
|
def test_the_migrations_patterns_anchor_to_a_word_start_the_postgres_way():
|
|
"""`\\m`, never `\\b` — the two are unrelated in Postgres."""
|
|
import pathlib
|
|
|
|
src = (pathlib.Path(__file__).resolve().parents[1]
|
|
/ "alembic" / "versions"
|
|
/ "0099_scrub_secrets_from_retrieval_logs.py").read_text()
|
|
|
|
for name in ("_TOKEN", "_ASSIGNED"):
|
|
line = src.split(f"{name} = (")[1].split(")")[0]
|
|
assert r"\m" in line, (
|
|
f"migration 0099's {name} no longer anchors to a word start. "
|
|
f"Without it a vendor prefix matches INSIDE a word — `sk-` in "
|
|
f"`task-notification` is the case that actually happened — and "
|
|
f"the UPDATE overwrites the only copy of the text it mangles."
|
|
)
|
|
assert r"\b" not in line, (
|
|
f"migration 0099's {name} uses `\\b`, which is a BACKSPACE in "
|
|
f"Postgres ARE rather than a word boundary. Python's `\\b` and "
|
|
f"Postgres's `\\m` look interchangeable and are not."
|
|
)
|