fix(service): stop secret redaction from destroying records - #31
Merged
Conversation
Value-pattern redaction replaced the WHOLE string on any match, and the
patterns were unanchored. "sk-" matched the middle of ordinary words
("task-", "disk-", "risk-", "brisk-") and "Bearer" matched the English
phrase "Bearer token", so ordinary ledger data was destroyed: paths like
".../work-task-sessions-4801fa" were stored as a bare placeholder. Worse,
every destroyed section_id collapsed onto the same literal string, merging
unrelated jobs into one phantom section. All of it recorded success with no
warning, which is exactly the quietly-wrong record this ledger exists to
avoid.
Now:
* patterns are anchored -- a word boundary before "sk-", and something
credential-shaped (>=16 chars of token charset containing a digit or a
separator) after "Bearer", so prose can no longer trip them;
* only the matched span is replaced, with a distinct
"[REDACTED_SECRET]" placeholder, so surrounding prose, ids and paths
survive;
* a redaction stamps server-authored metadata naming each field and its
pattern class, following the existing reserved-provenance convention:
value_redaction_applied / value_redaction_fields, with any
caller-supplied copy stripped first
(reserved_value_redaction_provenance_stripped) so an agent cannot claim
a redaction that never happened. The marker is never written in-band
inside the value, which would itself be redactable.
Sensitive key NAMES are unchanged: those still drop the whole value,
because that is a decision about the field rather than a content match.
Two existing tests asserted the old whole-string result. Both are updated
to the stronger assertion (exact new value, prose preserved); their
"secret is gone" assertions were left as-is and still pass.
Already-damaged historical values are unrecoverable and are deliberately
not backfilled.
Review of the span-redaction change found four defects, three of them the same failure class the change was made to end. The bearer credential run had been narrowed from a delimiter to a fixed [A-Za-z0-9._~+/=-] alphabet, so a real credential stopped at its first "%", ":", "|", "@", "#" or "&". "Bearer abc%2Fdefghijklmnopqrstuvwx" in a plain "summary" field matched only "abc", fell under the length floor, and the whole credential was written to the ledger -- the value pattern is the only defence there, because the key name is not credential-ish. The run is delimiter-based again (whitespace, comma, semicolon, quotes, brackets/braces/ parens end it), so the whole credential is taken. The discriminator was "contains a digit OR a separator", and "-" is a separator, so hyphenated engineering prose was still destroyed: "switch to Bearer token-based-authentication soon" became "switch to [REDACTED_SECRET] soon". It now requires a DIGIT. A digit is the one thing an English compound cannot contain, and JWT, base64, hex and UUID credentials all have one. The accepted residual is a >=16 character all-alphabetic credential, which the value patterns miss; a field NAMED like a credential still loses its whole value. A rare miss beats routinely shredding prose. Trusted session observations stamped the marker and then threw it away: the allowlist rebuild in mark_trusted_local_session_observation_event keeps only known metadata keys. A title carrying a credential was stored correctly redacted with nothing saying data had been cut, which defeats the point. The two server-authored keys join that allowlist. They cannot be forged -- redact_event_secrets strips caller copies before the rebuild -- and because the row now carries them into _prepare_recorded_event, that path takes an already_redacted flag so the redactor cannot strip its own marker as if a caller had planted it. _prepare_recorded_event redacted and stamped, then unconditionally assigned event_id/created_at, so a marker could name a field now holding an ordinary server-generated id with no placeholder in it. Rows naming a server-assigned field are dropped, and the marker with them if nothing else was cut. The prune also runs before the observation allowlist rebuild, which is where observation_revision is computed, so the stored digest keeps agreeing with the stored content. Also closed while in the area: strip_value_redaction_provenance cleared a forged marker only from metadata, but the stamp lands top-level for off-contract metadata, so a caller could plant one there and keep it. Both homes are cleared now, which is what the surrounding comment already claimed.
Three shipped bearer discriminators in a row destroyed ordinary engineering prose, each guessed rather than measured, each failing in the same direction. The last one required a DIGIT in the run after "Bearer" and asserted in a comment that "a digit is the one thing an English compound cannot contain" -- which is wrong for the exact vocabulary this ledger records: oauth2, sha256, base64, x509, rfc7519, http2. It cut "Bearer oauth2-client-credentials" and "debug Bearer sha256-hmac-signature", and newly mangled "Bearer $CI_API_TOKEN_V2" and "Bearer https://auth.example.com/v2/token". Calibrated instead against the maintainer's own store: 26773 distinct string values, 38951 distinct tokens, 8454 of them prose-shaped word compounds. The rule now refuses a shell variable reference, a URL, and any run that is entirely a word compound (words with an optional trailing version number joined by - _ . / = :), then accepts only a real Authorization header value, a >=16 character run carrying a separator no word compound reaches, a separator-free alphanumeric run of >=32, a UUID, or a run of >=10 consecutive digits. Measured zero false positives on all 26773 real values, all 8454 prose compounds spliced after "Bearer", and 49 adversarial phrases; zero misses on 13 credential shapes. The header branch captures the header NAME as `keep` and puts it back, so "Authorization: Bearer <jwt>" loses the credential and not the word "Authorization". The comment now states the discriminator that is actually implemented and names the shapes it knowingly does not catch. Both sk- patterns were re-measured against the same corpus and match nothing in it, so they are unchanged. No existing test was edited.
Three defects in the shipped bearer rule, each measured against the maintainer's store rather than argued. A COVERAGE REGRESSION first. `ghp_...`, `gho_/ghu_/ghs_/ghr_`, `github_pat_`, `glpat-`, `AKIA`/`ASIA`, `xox[abprs]-` and `AIza` are a short letter prefix joined to an alphanumeric body, which is precisely the word-compound shape the prose refusal exists to protect, so after a bare "Bearer" they stopped being redacted -- while main and the previous commit on this branch both cut them, and while they remain the credentials most likely to actually leak. They are now matched ahead of the refusals. `AKIA`, `ASIA` and `AIza` are matched case-sensitively inside the otherwise case-insensitive pattern. A PROSE FALSE POSITIVE second. The word-compound recogniser only accepted pieces shaped letters-then-optional-digits, so any run with a bare numeric piece was read as a credential: "Bearer rfc6750/section-2.1 says the header is required" was cut, and RFC 6750 is the Bearer Token specification itself. Pieces may now also be a bare 1..4 digit number; the cap keeps the >=10-digit credential branch alive. The Authorization-header branch was not the backstop the comment implied, because the prose refusals were applied to it as well: "Authorization: Bearer abcdefgh-ijklmnop-qrstuvwx" was kept verbatim. Nobody writes an HTTP header in the middle of a sentence, so the refusals now stop at the bare-"Bearer" branch and the header branch takes any >=16 character run. And the previous commit's own honesty. It claimed zero false positives on "all 8454 prose compounds spliced after Bearer", but only the tokens that were ALREADY word compounds were tested in that position. Splicing the FULL distinct token set gives the real numbers, which are now in the comment: 26773 distinct string values (27401 counting object keys) and 43213 distinct tokens; zero of those strings is touched as it stands; splicing all 43213 after "Bearer " cuts 6252, of which 5306 are opaque-id shapes and paths the rule cannot tell from a credential, 74 are non-ASCII, digit-led or punctuation-led fragments, and 98 are genuinely prose-shaped. Those 98 are now named in KNOWN GAPS instead of rounded away, including "Bearer instrumentation." -- a long word whose trailing sentence period leaves an empty compound piece. Against the shipped HEAD the same measurement gives 155 prose-shaped hits, so the two rule changes heal 57 and introduce none. Measured through the shipped _redact_secret_spans: 0 whole-string false positives over 27401 strings, 0 new splice false positives from any of the three changes, 29 of 29 pinned credential shapes cut, 51 of 51 pinned prose phrases kept byte for byte, replacement still idempotent, worst case 1.97ms on a 20000-character run. Three tests added, one per fix, each failing without it. No existing test was edited.
One `Bearer`-anchored pattern was doing three jobs with three different
precision requirements, so every guard added for one job silently disabled
another: the prose refusal that saves "Bearer oauth2-client-credentials" also
swallowed `ghp_...`, and applying it inside `Authorization:` kept a real
hyphen-joined token verbatim. Four rounds of widen-then-narrow all landed on
that one structure. It is now three separate compiled patterns that share no
refusal, so adding a family, or a prefix to a family, cannot weaken another.
1. PREFIXED PROVIDER TOKENS, matched ANYWHERE in a value rather than only after
"Bearer", with no prose refusal -- a vendor prefix plus a long body is not
prose. Adds `hf_`, `r8_` and `tok_`, which main cut and which both earlier
commits on this branch kept. Bodies are the vendor's alphabet rather than one
shared permissive charset: `hf_`/`r8_`/`tok_` take no `_`, so
"tok_rotation_is_documented_here" is not a credential, and `AKIA`/`ASIA` take
exactly 16 uppercase alphanumerics with a boundary after, so
"ASIAPACIFICREGIONCODE" is not one either. Both verified against the pattern.
2. HEADER CONTEXT, also with no prose refusal -- nobody writes an HTTP header
mid-sentence. Now covers the JSON-serialized `{"Authorization": "Bearer ..."}`
the reviewer found regressed, where the quote between the name and the colon
ended the match and the credential was stored. The header NAME is still
re-emitted and only the credential replaced.
3. BARE `Bearer <token>` IN PROSE, carrying the calibrated refusals unchanged.
This family is allowed to miss things, because 1 and 2 now cover the
realistic leak paths: a token pasted into a log or env line carries its own
prefix, and a token in transit carries its header.
THREE HONESTY DEFECTS, all of them verified before being fixed.
KNOWN GAPS said the separator-free miss window was "16..31 characters". It is
not: "Bearer " + "a"*32 is kept and "a"*33 is the first cut, and "Bearer " +
"a"*32 + "2024" (36 chars) is kept, because the recogniser reads a run as ONE
word when it is <=32 letters plus <=4 digits. The paragraph now states the rule
and quotes the five strings that prove it, and
test_the_separator_free_miss_window_is_where_known_gaps_says_it_is pins all
five so it cannot drift off the code again.
The provider-token fixtures pinned 3 of their 12 prefixes, not 12: 9 embedded
"0" * 24 and passed through the generic >=10-digit branch, so deleting the
provider family left them green. Measured, with the family removed: 3 of 12
failed. Every body is now purely alphabetic -- no 10-digit run, no >=32
alphanumeric run, no UUID, no `. / + = % : @ | # &` -- so each row can only be
matched by its own prefix. With the family removed, 15 of 15 now fail.
The residual breakdown could not be reproduced from the comment's own prose, so
the sub-counts are gone. One re-derivable number replaces them, with the exact
test written out: 24 of the 6252 spliced cuts are prose-shaped by
`^[A-Za-z]+(?:[-'][A-Za-z]+)*[.,:;!?]?$`. The rest are named qualitatively and
the reader is told to re-measure rather than quote.
MEASURED through the shipped compiled patterns against the maintainer's store
(6261 event lines; 26773 distinct string values, 27401 counting object keys,
43213 distinct whitespace tokens over the key-inclusive set), read-only:
- Whole-string false positives: 0 of 26773. Also 0 at main, at 71bf462 and at
2df8d1f.
- Splicing all 43213 tokens after "Bearer ": 6252 cut, and that set is
EXACTLY the set 2df8d1f cuts -- set difference in both directions is empty.
Family 3's set alone is likewise identical to 2df8d1f's single bearer_token
set, which is the proof that the split neither weakened nor widened it.
main cuts all 43213.
- Marginal contribution of the two new families on the real corpus: 0 values
and 0 spliced tokens each. They pay for themselves purely in shape coverage.
- Credential-shape coverage, a 146-case catalogue over six serializations
(bare Bearer, env/log line with no Bearer at all, header, JSON header,
`Authorization=`, curl `-H`): 142 cut here, 128 at main, 114 at 2df8d1f,
62 at 71bf462. Nothing cut at 2df8d1f is missed here (0 regressions), and 18
cases cut here are missed at main -- every provider token with no Bearer
word anywhere, which is how these actually leak.
- Coverage below main in 4 of 146: a 20-letter run, a 28-letter run, and
hyphen- and dot-joined 26-character opaque runs, each after a BARE "Bearer".
All four are cut here in all five header serializations; main caught them
only by cutting every run after the word, which is the incident this rule
exists to undo. Named in KNOWN GAPS with that reason.
- Idempotency: re-redacting every string any family touches is a no-op.
- Backtracking: adversarial repetitions of each family's ambiguous shape
(`Bearer a-1.` xN in a header, `Bearer abcd-abcd-...` xN, `ghp_` xN, nested
`{"Authorization": ` xN) scale linearly to N=16000, worst 20 ms.
Full suite: 2931 passed, 1 skipped. No existing test was edited to pass; the
provider fixture bodies were strengthened, and three tests added.
… gaps Five review findings, all of them either a number the comment could not support or a coverage class it left unnamed. The >=10-digit branch was justified with "corpus digit runs top out at 8 digits". That is false: re.finditer(r"[0-9]+") over the snapshot's 27105 values finds 131131 runs, 414 of them >=10 digits, longest 19. Replaced with the branch's measured price -- deleting the digit alternative moves the spliced cut set 6405 -> 6178, so it alone accounts for 227 of 43545 spliced tokens, all opaque agent ids -- and whole-string stays 0 either way. Family 2 carried a >=16-character floor copied from family 3, so a short credential inside a real Authorization header was kept while main cut it (15 chars kept, 16 cut; pure digits kept at 8..15). Removing it outright is not safe: the pinned prose corpus contains "Authorization: Bearer token is required for all endpoints" and "the Authorization: Bearer header must be present on every request". The floor is now 8, the smallest value that clears every run in that corpus and closes the whole 8..15 window. Whole-string cuts stay 0 at every floor from 16 to 1; header-splice coverage goes 22445 -> 32709 of 43545. Family 1 promised that a token in a log or env line carries its own prefix, which was false for six vendors both main and HEAD kept in an env line, a log line and a URL query: sk_live_, sk_test_, npm_, pypi-, shpat_, sq0atp-. Added, each measured at 0 matches over the corpus first. pypi- is anchored on the macaroon body (AgE) because the bare prefix cuts the real ledger value "pypi-publish-and-install-smoke". KNOWN GAPS is now exhaustive against a 336-case catalogue scored on whether the credential is GONE, not on whether the string changed. That distinction found a gap the old wording would have hidden: a Key=Value;Key=Value connection string loses only its first pair, so its AccountKey survives even inside a header. Named as (c), with a test that asserts the secret is still there rather than pretending otherwise. The 20 ms ReDoS figure is gone; a wall-clock number does not reproduce. The claim is now linearity, with the measured doubling ratios and the sweep to re-run. Six new tests, no existing test edited. Suite 2937 passed, 1 skipped.
A reviewer's independently built catalogue immediately found vendor formats this module keeps and main cut, which is what an enumeration of vendor prefixes will always do. Three corrections, no behaviour guesswork: - Drop the "EXHAUSTIVE for the catalogue" claim. The gaps paragraph now says what shapes are missed and why, plus how to re-derive the set for a given catalogue, instead of asserting a census it cannot support. - Gap (a) quoted only the single-piece bound (32 letters + 4 digits) and so understated the miss window by an unbounded factor: a compound of such pieces has no ceiling, and a 101-character hyphen-joined run is kept. - The preamble said six of seven gaps are still cut in a header. (d) is not: "$TOKEN" is 6 run-characters and family 2's floor is 8. Corrected, and gap (c) no longer calls itself the only header-proof gap. Also adds lin_api_, dop_v1_ and rk_live_/rk_test_ to family 1, each measured at 0 false positives over the 27739 distinct strings in the real store.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Secret redaction was destroying the record it was meant to protect, silently, with a success response.
_redact_secretsreplaced the entire string value with[REDACTED]whenever any pattern matched anywhere in it, and the patterns were unanchored — the api-key pattern matched inside ordinary words (task-,disk-,risk-), and the bearer pattern matched the English phrase "Bearer token". On the maintainer's own live ledger this had already destroyed 117 project paths, 20 section ids, 5 summaries and 2 idempotency keys. Because every casualty collapsed onto the same literal[REDACTED], three unrelated jobs across two different projects had merged into one phantom section in the work ledger.How
Redaction now replaces only the matched span, and every redaction records which field was cut and which pattern class cut it (server-authored metadata, following the existing reserved-provenance convention). A repair you cannot see is a bug.
The patterns are split into three independent families so a guard added for one can never disable another again (four earlier rounds oscillated because one
Bearer-anchored pattern was doing three jobs):Bearerword.Authorization/Proxy-Authorizationheaders — including their JSON andcurl -Hserializations.Bearer <token>in prose — the only genuinely ambiguous case, and the only one carrying prose refusals.Family 3's discriminator was calibrated against the real ledger, not guessed: zero false positives across ~27k distinct strings and ~43k tokens. Coverage is now broader than before this change, not narrower (142 of a 146-shape catalogue vs main's 128). The shapes still missed are named in the module with the method to re-derive them — no "exhaustive" claim, because a vendor-prefix list can never be.
Verification
.venv/bin/pytest -q): 2937 passed, 1 skipped.tests/test_secret_value_redaction.py(33 tests) pins: real secrets still cut in every shape (mid-sentence, nested, in lists/tuples); the incident false positives survive byte-for-byte; twotask-…-shaped ids stay distinct; the visibility marker is stamped only when something was cut and cannot be forged by a caller.