From 0ee8ba2ff598437887bc59b0a455691e87cc3ac1 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 21 Sep 2026 21:31:00 +0530 Subject: [PATCH 1/7] fix(redaction): redact credentials split by an invisible character Every shape matcher in RedactString describes a contiguous run of body characters, and none of their classes admits a control or format character. One NUL or ESC dropped into the body of a key therefore ended the match and both fragments were emitted untouched. Whoever read that output next saw the original key back, because nothing about those characters was ever displayed: a terminal eats the escape, a log viewer or JSON consumer strips the control byte, and a copy-paste drops both. The matchers now also run over a copy with those characters removed, and the spans they find are mapped back so the replacement covers the original bytes, separators included. Emitting the compacted copy would hand the caller a different string than the one they passed in, and leaving a separator behind would read as two secrets where there was one. Written against the character classes rather than the two bytes in the report, because every other Cc and Cf character behaves the same way: BEL, backspace, DEL, the C1 controls, zero-width space and joiner, word joiner, BOM and soft hyphen all split a key on main. Tab, newline and carriage return are deliberately excluded and a test says so: they are real text structure, a credential broken across a line is visible to whoever reads it rather than hidden from them, and stripping them would let one match span a line break and replace unrelated lines. Text with no such character takes exactly the path it took before, decided by a byte scan that allocates nothing, so the common case is unchanged at 51 allocations. Text that carries control characters and no credential pays for one compacted copy and a second pattern pass. Fixes #969 --- internal/redaction/control_split.go | 198 +++++++++++++++++++++++ internal/redaction/control_split_test.go | 188 +++++++++++++++++++++ internal/redaction/redaction.go | 7 +- 3 files changed, 391 insertions(+), 2 deletions(-) create mode 100644 internal/redaction/control_split.go create mode 100644 internal/redaction/control_split_test.go diff --git a/internal/redaction/control_split.go b/internal/redaction/control_split.go new file mode 100644 index 000000000..590c40f87 --- /dev/null +++ b/internal/redaction/control_split.go @@ -0,0 +1,198 @@ +package redaction + +import ( + "sort" + "strings" + "unicode" + "unicode/utf8" +) + +// A CREDENTIAL IS STILL A CREDENTIAL WITH AN INVISIBLE BYTE IN THE MIDDLE. +// +// The shape matchers below (openaiKeyPattern, textSecretPatterns) all describe a +// CONTIGUOUS run of body characters, and none of their character classes admits +// a control or format character. So a single NUL, ESC or zero-width space +// dropped into the body of a key ends the match, and RedactString emits the two +// fragments untouched. Whoever reads that output next — a terminal that eats the +// escape, a log viewer, a JSON consumer that strips control bytes, or a person +// copying the text — sees the original key back, because nothing about those +// characters was ever displayed. Reported in #969 for NUL and ESC; every other +// Cc and Cf character behaves the same way, which is why this is written against +// the classes rather than against the two bytes named there. +// +// The rule is normalize-then-match, applied so that the ORIGINAL bytes are what +// gets replaced: matching on a compacted copy and then redacting that copy would +// hand back a string missing the separators, which is a different string from +// the one the caller passed in. So the spans are mapped back and the original +// slice, separators included, is what the replacement covers. +// +// WHAT THIS DELIBERATELY DOES NOT STRIP: tab, newline and carriage return. +// Those three are real text structure rather than invisible filler, so a +// credential broken across a line boundary is still two visible fragments here +// and is not covered. Stripping them would also let a match span a line break +// and collapse unrelated lines into one replacement. A credential split that way +// is visible to whoever reads it; the ones handled here are not. + +// splitSecretSeparator reports a character that can sit inside a credential +// without being seen. Cc covers the C0 and C1 controls (NUL, ESC, DEL, NEL, CSI) +// and Cf the format characters (zero-width space and joiner, word joiner, BOM, +// soft hyphen). Tab, newline and carriage return are excluded: see above. +func splitSecretSeparator(r rune) bool { + switch r { + case '\t', '\n', '\r': + return false + } + return unicode.Is(unicode.Cc, r) || unicode.Is(unicode.Cf, r) +} + +// containsSplitSeparator is the cheap gate in front of the compaction, so text +// with nothing invisible in it — which is nearly all text — costs one scan and +// no allocation. ASCII is settled from the byte alone; only a byte that could +// begin a multi-byte rune needs decoding, and the C1 and Cf blocks all start +// with a lead byte at or above 0xC2. +func containsSplitSeparator(value string) bool { + for i := 0; i < len(value); i++ { + b := value[i] + if b < utf8.RuneSelf { + if b < 0x20 || b == 0x7f { + if b == '\t' || b == '\n' || b == '\r' { + continue + } + return true + } + continue + } + r, width := utf8.DecodeRuneInString(value[i:]) + if splitSecretSeparator(r) { + return true + } + i += width - 1 + } + return false +} + +// compactSplitSeparators returns value with every splitSecretSeparator removed. +// +// Decoded explicitly rather than with range, because an invalid byte must be +// measured as the one byte it occupies. Ranging reports it as U+FFFD, whose +// encoded width is three, and the offsets this feeds have to stay in the +// source's own bytes. +func compactSplitSeparators(value string) string { + var builder strings.Builder + builder.Grow(len(value)) + for offset := 0; offset < len(value); { + r, width := utf8.DecodeRuneInString(value[offset:]) + if !splitSecretSeparator(r) { + builder.WriteString(value[offset : offset+width]) + } + offset += width + } + return builder.String() +} + +// splitSeparatorIndex gives, for each byte of compactSplitSeparators(value), the +// start and end offsets in value of the rune it came from. That is what lets a +// match found in the compacted text name the exact original slice it stands for. +// +// Built only once a match exists. Text carrying control characters and no +// credential is the ordinary case for terminal output, and it should not pay for +// a table nothing will read. +func splitSeparatorIndex(value string) (starts, ends []int) { + starts = make([]int, 0, len(value)) + ends = make([]int, 0, len(value)) + for offset := 0; offset < len(value); { + r, width := utf8.DecodeRuneInString(value[offset:]) + if splitSecretSeparator(r) { + offset += width + continue + } + for i := 0; i < width; i++ { + starts = append(starts, offset) + ends = append(ends, offset+width) + } + offset += width + } + return starts, ends +} + +// shapeSecretSpans returns the byte ranges of value claimed by the +// credential-shape matchers, applying the same openai filter RedactString uses +// so the two agree on what a key is. +func shapeSecretSpans(value string) [][]int { + var spans [][]int + for _, span := range openaiKeyPattern.FindAllStringIndex(value, -1) { + if !openAIShapeIsSecret(value[span[0]:span[1]]) { + continue + } + spans = append(spans, span) + } + for _, pattern := range textSecretPatterns { + spans = append(spans, pattern.FindAllStringIndex(value, -1)...) + } + return spans +} + +// openAIShapeIsSecret is the digit/kebab-case filter RedactString applies to an +// openaiKeyPattern match, named so both callers state the same rule once. +func openAIShapeIsSecret(match string) bool { + if knownOpenAIKeyPrefix(match) || secretMatchHasDigit(match) { + return true + } + return !strings.Contains(strings.TrimPrefix(match, "sk-"), "-") +} + +// redactControlSplitSecrets replaces credentials whose body is interrupted by +// invisible characters. It is a no-op for text containing none of them, which is +// nearly all text, so the contiguous path below keeps its behavior exactly. +func redactControlSplitSecrets(value, replacement string) string { + if !containsSplitSeparator(value) { + return value + } + compact := compactSplitSeparators(value) + if len(compact) == len(value) { + return value + } + spans := shapeSecretSpans(compact) + if len(spans) == 0 { + return value + } + starts, ends := splitSeparatorIndex(value) + mapped := make([][]int, 0, len(spans)) + for _, span := range spans { + if span[0] >= len(starts) || span[1] <= span[0] || span[1] > len(ends) { + continue + } + mapped = append(mapped, []int{starts[span[0]], ends[span[1]-1]}) + } + return spliceSpans(value, mapped, replacement) +} + +// spliceSpans replaces every named range of value with replacement, merging +// ranges that overlap or touch so one credential cannot be replaced twice. +func spliceSpans(value string, spans [][]int, replacement string) string { + if len(spans) == 0 { + return value + } + sort.Slice(spans, func(i, j int) bool { + if spans[i][0] != spans[j][0] { + return spans[i][0] < spans[j][0] + } + return spans[i][1] > spans[j][1] + }) + var builder strings.Builder + builder.Grow(len(value)) + written := 0 + for _, span := range spans { + if span[0] < written { + if span[1] <= written { + continue + } + span[0] = written + } + builder.WriteString(value[written:span[0]]) + builder.WriteString(replacement) + written = span[1] + } + builder.WriteString(value[written:]) + return builder.String() +} diff --git a/internal/redaction/control_split_test.go b/internal/redaction/control_split_test.go new file mode 100644 index 000000000..1805e13c1 --- /dev/null +++ b/internal/redaction/control_split_test.go @@ -0,0 +1,188 @@ +package redaction + +import ( + "strings" + "testing" +) + +const ( + awsKey = "AKIAIOSFODNN7EXAMPLE" + anthropicKey = "sk-ant-api03-aaaaaaaaaaaaaaaaaaaaaaaa0123456789ABCD" + githubKey = "ghp_cccccccccccccccccccccccccccccccccccc" + openaiKey = "sk-proj-bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" +) + +// rejoin drops every character a reader would never see, which is what a +// terminal, a log viewer or a copy-paste does to this output. If the secret is +// back after that, the redaction did not happen. +func rejoin(value string) string { + return strings.Map(func(r rune) rune { + if splitSecretSeparator(r) { + return -1 + } + return r + }, value) +} + +// AN INVISIBLE BYTE INSIDE A KEY MUST NOT END THE MATCH. +// +// Every shape matcher describes a contiguous body, so one NUL or ESC in the +// middle of a key ended the match and both fragments were emitted. Dropping the +// byte — which is what the next reader does, because it was never displayed — +// put the original key back. Reported in #969 for NUL and ESC; this covers the +// classes those two belong to, because every member behaves the same way. +func TestRedactStringRedactsSecretsSplitByInvisibleCharacters(t *testing.T) { + separators := map[string]string{ + "NUL": "\x00", + "ESC": "\x1b", + "BEL": "\x07", + "backspace": "\x08", + "vertical tab": "\x0b", + "form feed": "\x0c", + "DEL": "\x7f", + "C1 NEL": "\u0085", + "C1 CSI": "\u009b", + "zero width space": "\u200b", + "zero width joiner": "\u200d", + "word joiner": "\u2060", + "byte order mark": "\ufeff", + "soft hyphen": "\u00ad", + } + // Every entry has to BE a separator, or its leg passes vacuously: inserting an + // ordinary character into a key also stops the secret matching, and the rejoin + // below would then find nothing. An escape mangled on the way into this file is + // exactly how that happens. + for name, separator := range separators { + if runes := []rune(separator); len(runes) != 1 || !splitSecretSeparator(runes[0]) { + t.Fatalf("SETUP INVALID: %s is %q, which is not a single invisible separator", name, separator) + } + } + secrets := map[string]string{"aws": awsKey, "anthropic": anthropicKey, "github": githubKey, "openai": openaiKey} + for secretName, secret := range secrets { + // The premise: the unsplit form already redacts, so a failure below is + // about the separator and not about the shape being unknown. + if out := RedactString(secret, Options{}); strings.Contains(out, secret) { + t.Fatalf("SETUP INVALID: the unsplit %s key is not redacted at all: %q", secretName, out) + } + for sepName, separator := range separators { + for _, at := range []int{1, len(secret) / 2, len(secret) - 1} { + split := secret[:at] + separator + secret[at:] + out := RedactString(split, Options{}) + if got := rejoin(out); strings.Contains(got, secret) { + t.Errorf("%s key split by %s at byte %d survives: %q rejoins to %q", secretName, sepName, at, out, got) + } + } + } + } +} + +// The replacement has to cover the ORIGINAL bytes, separators included. Leaving +// the separator behind would emit `[REDACTED]\x00[REDACTED]` and tell a reader +// there were two secrets, and leaving either fragment behind is the leak itself. +func TestRedactStringCoversTheSeparatorsInsideASplitSecret(t *testing.T) { + for _, testCase := range []struct { + name string + value string + want string + }{ + {"one separator", "key=" + awsKey[:8] + "\x00" + awsKey[8:], "key=[REDACTED]"}, + {"several separators", "key=" + strings.Join(strings.Split(awsKey, ""), "\x1b"), "key=[REDACTED]"}, + {"adjacent separators", "key=" + awsKey[:8] + "\x00\x1b\u200b" + awsKey[8:], "key=[REDACTED]"}, + {"two split secrets", awsKey[:8] + "\x00" + awsKey[8:] + " and " + githubKey[:8] + "\x00" + githubKey[8:], "[REDACTED] and [REDACTED]"}, + {"separator before the secret", "\x00" + awsKey, "\x00[REDACTED]"}, + {"separator after the secret", awsKey + "\x00", "[REDACTED]\x00"}, + {"text around it survives", "before " + awsKey[:4] + "\x00" + awsKey[4:] + " after", "before [REDACTED] after"}, + } { + t.Run(testCase.name, func(t *testing.T) { + if got := RedactString(testCase.value, Options{}); got != testCase.want { + t.Errorf("RedactString(...) = %q, want %q", got, testCase.want) + } + }) + } +} + +// NOTHING ELSE MOVES. The compaction exists to find a match; it must never +// reach text that has no secret in it, and text with no invisible character at +// all must take exactly the path it took before. +func TestRedactStringLeavesOrdinaryTextWithControlCharactersAlone(t *testing.T) { + for _, value := range []string{ + "a plain sentence", + "a sentence\x00with a NUL in it", + "\x1b[31mcolored output\x1b[0m", + "tab\tseparated\tcolumns", + "line one\nline two\r\nline three", + "zero\u200bwidth\u200bspaces", + "", "\x00", "\x00\x00\x00", + "not-a-secret-just-a-long-kebab-case-identifier-here", + "sk-some-kebab-case-value-without-any-digits-at-all", + } { + if got := RedactString(value, Options{}); got != value { + t.Errorf("RedactString(%q) = %q, want it unchanged", value, got) + } + } +} + +// The openai filter that keeps kebab-case identifiers out of the redactor has +// to apply on the split path too, or working around the false positive would +// become a matter of inserting a control byte. +func TestSplitPathKeepsTheOpenAIKebabCaseFilter(t *testing.T) { + kebab := "sk-some-kebab-case-value-without-digits" + split := kebab[:6] + "\x00" + kebab[6:] + if got := RedactString(split, Options{}); got != split { + t.Errorf("a kebab-case identifier split by a NUL was redacted: %q", got) + } + // ... while a real key of the same family, split the same way, is not spared. + realKey := openaiKey[:6] + "\x00" + openaiKey[6:] + if got := RedactString(realKey, Options{}); strings.Contains(rejoin(got), openaiKey) { + t.Errorf("a real openai key split by a NUL survived: %q", got) + } +} + +// TAB, NEWLINE AND CARRIAGE RETURN ARE OUT OF SCOPE ON PURPOSE, and this says +// so in a place that fails if someone changes it without meaning to. They are +// real text structure: stripping them would let one match span a line break and +// replace unrelated lines, and a credential broken across a line is visible to +// whoever reads it rather than hidden from them. +func TestLineStructureIsNotTreatedAsAnInvisibleSeparator(t *testing.T) { + for name, separator := range map[string]string{"tab": "\t", "newline": "\n", "carriage return": "\r"} { + if splitSecretSeparator([]rune(separator)[0]) { + t.Errorf("%s is treated as an invisible separator; if that is now wanted, this test and the note in control_split.go both need to change", name) + } + split := awsKey[:8] + separator + awsKey[8:] + if got := RedactString(split, Options{}); got != split { + t.Errorf("%s split was redacted: %q. That may be an improvement, but it is a scope change this test is here to make deliberate", name, got) + } + } +} + +// Malformed UTF-8 must not move the span mapping, which works in the source's +// own byte offsets. A decoder that measured an invalid byte as the three bytes +// of U+FFFD would shift every later span. +func TestSplitRedactionHandlesMalformedUTF8(t *testing.T) { + for _, testCase := range []struct { + name string + value string + }{ + {"invalid byte before the secret", "\xff" + awsKey[:8] + "\x00" + awsKey[8:]}, + {"invalid byte inside the split", awsKey[:8] + "\x00\xff" + awsKey[8:]}, + {"invalid byte after the secret", awsKey[:8] + "\x00" + awsKey[8:] + "\xfe"}, + {"only invalid bytes", "\xff\xfe\x00\xff"}, + } { + t.Run(testCase.name, func(t *testing.T) { + got := RedactString(testCase.value, Options{}) + if strings.Contains(rejoin(got), awsKey) { + t.Errorf("the key survived: %q", got) + } + }) + } +} + +// The compaction is only a lens for finding the match. A string with no +// separator at all must come back byte-identical, including its invalid bytes. +func TestCompactionDoesNotRewriteTextItDoesNotRedact(t *testing.T) { + for _, value := range []string{"plain", "with\xffinvalid", "tab\there", "\xef\xbb\xbfbom at the front"} { + if got := redactControlSplitSecrets(value, RedactedSecret); got != value { + t.Errorf("redactControlSplitSecrets(%q) = %q, want it unchanged", value, got) + } + } +} diff --git a/internal/redaction/redaction.go b/internal/redaction/redaction.go index e65312821..836bcdcc8 100644 --- a/internal/redaction/redaction.go +++ b/internal/redaction/redaction.go @@ -224,11 +224,14 @@ func RedactString(value string, options Options) string { } return parts[1] + parts[2] + "=" + replacement }) + // Credentials broken up by invisible characters are redacted before the + // contiguous matchers run, because those matchers cannot see them at all and + // would leave the fragments in place. No-op for text with no such character. + redacted = redactControlSplitSecrets(redacted, replacement) // openai keys first so the filter can drop kebab-case false positives // before any other pattern rewrites nearby text. redacted = openaiKeyPattern.ReplaceAllStringFunc(redacted, func(match string) string { - if !knownOpenAIKeyPrefix(match) && !secretMatchHasDigit(match) && - strings.Contains(strings.TrimPrefix(match, "sk-"), "-") { + if !openAIShapeIsSecret(match) { return match } return replacement From 5dbf5e45af5cbcfbef0472ebdd02acec71a93cc4 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 21 Sep 2026 21:32:37 +0530 Subject: [PATCH 2/7] test(redaction): assert the exact output around a malformed byte The malformed-UTF8 legs only asked whether the key was gone, so a decoder that measured an invalid byte as the three bytes of U+FFFD still passed: the replacement covered the secret and moved its edges onto the bytes beside it. They now assert the whole string. One leg also records that an invalid byte is not one of the invisible separators, on the same ground as tab and newline: it decodes to U+FFFD and is drawn. --- internal/redaction/control_split_test.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/internal/redaction/control_split_test.go b/internal/redaction/control_split_test.go index 1805e13c1..be193c6f9 100644 --- a/internal/redaction/control_split_test.go +++ b/internal/redaction/control_split_test.go @@ -158,18 +158,32 @@ func TestLineStructureIsNotTreatedAsAnInvisibleSeparator(t *testing.T) { // Malformed UTF-8 must not move the span mapping, which works in the source's // own byte offsets. A decoder that measured an invalid byte as the three bytes // of U+FFFD would shift every later span. +// The exact output is asserted, not just the absence of the key. An invalid +// byte measured as three would still cover the secret while moving the +// replacement's edges onto the bytes around it, which reads as a pass if the +// test only asks whether the secret is gone. func TestSplitRedactionHandlesMalformedUTF8(t *testing.T) { for _, testCase := range []struct { name string value string + want string }{ - {"invalid byte before the secret", "\xff" + awsKey[:8] + "\x00" + awsKey[8:]}, - {"invalid byte inside the split", awsKey[:8] + "\x00\xff" + awsKey[8:]}, - {"invalid byte after the secret", awsKey[:8] + "\x00" + awsKey[8:] + "\xfe"}, - {"only invalid bytes", "\xff\xfe\x00\xff"}, + {"invalid byte before the secret", "\xff" + awsKey[:8] + "\x00" + awsKey[8:], "\xff[REDACTED]"}, + // An invalid byte is NOT one of the separators: it decodes to U+FFFD and + // is drawn, so a key broken by one is visible to whoever reads it, the + // same argument that keeps tab and newline out. It is kept in the + // compacted text and so it still ends the match. + {"invalid byte inside the split", awsKey[:8] + "\x00\xff" + awsKey[8:], awsKey[:8] + "\x00\xff" + awsKey[8:]}, + {"invalid byte after the secret", awsKey[:8] + "\x00" + awsKey[8:] + "\xfe", "[REDACTED]\xfe"}, + {"invalid bytes on both sides", "\xff\xfe" + awsKey[:8] + "\x00" + awsKey[8:] + "\xfd\xfc", "\xff\xfe[REDACTED]\xfd\xfc"}, + {"invalid byte then text then the secret", "\xffpre " + awsKey[:8] + "\x00" + awsKey[8:] + " post", "\xffpre [REDACTED] post"}, + {"only invalid bytes", "\xff\xfe\x00\xff", "\xff\xfe\x00\xff"}, } { t.Run(testCase.name, func(t *testing.T) { got := RedactString(testCase.value, Options{}) + if got != testCase.want { + t.Errorf("RedactString(%q) = %q, want %q", testCase.value, got, testCase.want) + } if strings.Contains(rejoin(got), awsKey) { t.Errorf("the key survived: %q", got) } From 4bed606b1a766ae7f00243c0d3b1bc09b788f93f Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 23 Sep 2026 09:49:51 +0530 Subject: [PATCH 3/7] fix(redaction): keep credential boundaries while matching across invisible characters The split-credential pass compacted every Cc and Cf character out of the whole string before matching, which removed delimiters as well as filler. Two things followed. A whole credential written after a NUL lost the word boundary the shape matchers open with, so "prefixAKIA......" stopped redacting even though the unsplit "prefixAKIA..." has always redacted. And with the delimiter between two credentials gone, the unbounded JWT shapes ran out of the first key and through the header of the next, replacing both with one marker and exposing the second token's payload and signature. The same reach consumed ordinary prose written after a key. Three changes, one rule: a separator is filler inside a credential and a delimiter outside one, and the difference comes from the source rather than from the compacted copy. The pass now runs after the contiguous matchers instead of before them, over what they left, and never across a replacement. A whole credential is claimed by its own strict match, so neighbours stay separate and the delimiter between them survives. The shape matchers used here drop their leading word boundary, and the byte before the match's original start is what has to be a non-word byte, so a key after a NUL redacts and a key glued to the end of a word still does not. The offset table is gone with them. It carried a start and an end for every source byte, which is sixteen bytes per byte of input on a 64-bit build, and RedactString is handed whole command output and notification bodies with no bound in front of it. Only the endpoints of actual matches are resolved now, in one walk. On a 4 MiB input with a split key, the split path allocated 223.6 MB before this change and 154.9 MB after, against 146.3 MB for the same text with no separator in it. Tests cover a delimiter in front of a split key against its unsplit verdict at every kind of leading byte, each supported shape placed in front of a JWT with each class of separator, text immediately behind a key, a caller-supplied replacement made of word characters, and the allocation ceiling. --- internal/redaction/control_split.go | 210 ++++++++++++++++++----- internal/redaction/control_split_test.go | 180 +++++++++++++++++++ internal/redaction/redaction.go | 9 +- 3 files changed, 355 insertions(+), 44 deletions(-) diff --git a/internal/redaction/control_split.go b/internal/redaction/control_split.go index 590c40f87..b817ad6c1 100644 --- a/internal/redaction/control_split.go +++ b/internal/redaction/control_split.go @@ -1,6 +1,7 @@ package redaction import ( + "regexp" "sort" "strings" "unicode" @@ -9,13 +10,13 @@ import ( // A CREDENTIAL IS STILL A CREDENTIAL WITH AN INVISIBLE BYTE IN THE MIDDLE. // -// The shape matchers below (openaiKeyPattern, textSecretPatterns) all describe a +// The shape matchers (openaiKeyPattern, textSecretPatterns) all describe a // CONTIGUOUS run of body characters, and none of their character classes admits // a control or format character. So a single NUL, ESC or zero-width space // dropped into the body of a key ends the match, and RedactString emits the two -// fragments untouched. Whoever reads that output next — a terminal that eats the +// fragments untouched. Whoever reads that output next, a terminal that eats the // escape, a log viewer, a JSON consumer that strips control bytes, or a person -// copying the text — sees the original key back, because nothing about those +// copying the text, sees the original key back, because nothing about those // characters was ever displayed. Reported in #969 for NUL and ESC; every other // Cc and Cf character behaves the same way, which is why this is written against // the classes rather than against the two bytes named there. @@ -26,6 +27,24 @@ import ( // the one the caller passed in. So the spans are mapped back and the original // slice, separators included, is what the replacement covers. // +// THE SAME CHARACTER IS FILLER INSIDE A CREDENTIAL AND A DELIMITER OUTSIDE ONE, +// and the difference cannot be read off the compacted text, so it is taken from +// the two places that do know it: +// +// - This pass runs AFTER the contiguous matchers, over their output, and never +// across a replacement. A credential that is whole gets claimed by its own +// strict match, so two whole credentials with a separator between them stay +// two replacements with the separator still between them, rather than one +// match reaching out of the first and into the second. +// - The leading word boundary is checked against the SOURCE, not against the +// joined text. A separator in front of a key is a boundary, and a whole key +// written after a NUL already redacts, so removing that NUL must not turn +// the key into the tail of the word in front of it. The patterns below drop +// their leading boundary, and the byte before the match's ORIGINAL start is +// what has to be a non-word byte instead. So "prefixAKIA......" +// redacts, because unsplit "prefixAKIA..." does, and "xAKIA......" +// does not, because unsplit "xAKIA..." does not either. +// // WHAT THIS DELIBERATELY DOES NOT STRIP: tab, newline and carriage return. // Those three are real text structure rather than invisible filler, so a // credential broken across a line boundary is still two visible fragments here @@ -46,10 +65,10 @@ func splitSecretSeparator(r rune) bool { } // containsSplitSeparator is the cheap gate in front of the compaction, so text -// with nothing invisible in it — which is nearly all text — costs one scan and -// no allocation. ASCII is settled from the byte alone; only a byte that could -// begin a multi-byte rune needs decoding, and the C1 and Cf blocks all start -// with a lead byte at or above 0xC2. +// with nothing invisible in it, which is nearly all text, costs one scan and no +// allocation. ASCII is settled from the byte alone; only a byte that could begin +// a multi-byte rune needs decoding, and the C1 and Cf blocks all start with a +// lead byte at or above 0xC2. func containsSplitSeparator(value string) bool { for i := 0; i < len(value); i++ { b := value[i] @@ -90,43 +109,58 @@ func compactSplitSeparators(value string) string { return builder.String() } -// splitSeparatorIndex gives, for each byte of compactSplitSeparators(value), the -// start and end offsets in value of the rune it came from. That is what lets a -// match found in the compacted text name the exact original slice it stands for. -// -// Built only once a match exists. Text carrying control characters and no -// credential is the ordinary case for terminal output, and it should not pay for -// a table nothing will read. -func splitSeparatorIndex(value string) (starts, ends []int) { - starts = make([]int, 0, len(value)) - ends = make([]int, 0, len(value)) - for offset := 0; offset < len(value); { - r, width := utf8.DecodeRuneInString(value[offset:]) - if splitSecretSeparator(r) { - offset += width - continue - } - for i := 0; i < width; i++ { - starts = append(starts, offset) - ends = append(ends, offset+width) - } - offset += width +// leadingWordBoundary is the token every shape matcher opens with. The split +// matchers drop it and check the source byte instead; see the header comment. +const leadingWordBoundary = `\b` + +// splitOpenAIPattern and splitTextPatterns are the shape matchers with their +// leading word boundary removed, derived from the originals rather than written +// out a second time so the two lists cannot drift apart. +var ( + splitOpenAIPattern = withoutLeadingBoundary(openaiKeyPattern) + splitTextPatterns = withoutLeadingBoundaries(textSecretPatterns) +) + +func withoutLeadingBoundary(pattern *regexp.Regexp) *regexp.Regexp { + source := pattern.String() + relaxed := strings.TrimPrefix(source, leadingWordBoundary) + if relaxed == source { + return pattern } - return starts, ends + return regexp.MustCompile(relaxed) +} + +func withoutLeadingBoundaries(patterns []*regexp.Regexp) []*regexp.Regexp { + relaxed := make([]*regexp.Regexp, 0, len(patterns)) + for _, pattern := range patterns { + relaxed = append(relaxed, withoutLeadingBoundary(pattern)) + } + return relaxed +} + +// wordByte is the class Go's regexp word boundary is defined over: ASCII +// letters, digits and underscore, and nothing else. A multi-byte lead or +// continuation byte falls outside it, which is what the boundary already +// assumes. +func wordByte(b byte) bool { + return b == '_' || + ('0' <= b && b <= '9') || + ('A' <= b && b <= 'Z') || + ('a' <= b && b <= 'z') } // shapeSecretSpans returns the byte ranges of value claimed by the -// credential-shape matchers, applying the same openai filter RedactString uses -// so the two agree on what a key is. +// boundary-relaxed credential shapes, applying the same openai filter +// RedactString uses so the two agree on what a key is. func shapeSecretSpans(value string) [][]int { var spans [][]int - for _, span := range openaiKeyPattern.FindAllStringIndex(value, -1) { + for _, span := range splitOpenAIPattern.FindAllStringIndex(value, -1) { if !openAIShapeIsSecret(value[span[0]:span[1]]) { continue } spans = append(spans, span) } - for _, pattern := range textSecretPatterns { + for _, pattern := range splitTextPatterns { spans = append(spans, pattern.FindAllStringIndex(value, -1)...) } return spans @@ -141,10 +175,103 @@ func openAIShapeIsSecret(match string) bool { return !strings.Contains(strings.TrimPrefix(match, "sk-"), "-") } +// splitSourceSpans maps match ranges found in compactSplitSeparators(value) +// back onto value, returning the original slice each one stands for. +// +// Only the endpoints of actual matches are resolved. An index over every source +// byte would cost at least sixteen bytes per byte of input on a 64-bit build, +// and RedactString is handed whole command output and notification bodies, so +// that table would be sized by the text rather than by the number of credentials +// in it. The walk below is a single pass whose memory is two ints per match +// endpoint. +func splitSourceSpans(value string, spans [][]int) [][]int { + if len(spans) == 0 { + return nil + } + wanted := make([]int, 0, 2*len(spans)) + for _, span := range spans { + if span[1] <= span[0] { + continue + } + wanted = append(wanted, span[0], span[1]-1) + } + if len(wanted) == 0 { + return nil + } + sort.Ints(wanted) + unique := wanted[:1] + for _, offset := range wanted[1:] { + if offset != unique[len(unique)-1] { + unique = append(unique, offset) + } + } + wanted = unique + + starts := make([]int, len(wanted)) + ends := make([]int, len(wanted)) + next := 0 + compact := 0 + for offset := 0; offset < len(value) && next < len(wanted); { + r, width := utf8.DecodeRuneInString(value[offset:]) + if splitSecretSeparator(r) { + offset += width + continue + } + for next < len(wanted) && wanted[next] < compact+width { + starts[next] = offset + ends[next] = offset + width + next++ + } + compact += width + offset += width + } + if next < len(wanted) { + return nil + } + + mapped := make([][]int, 0, len(spans)) + for _, span := range spans { + if span[1] <= span[0] { + continue + } + start := starts[sort.SearchInts(wanted, span[0])] + end := ends[sort.SearchInts(wanted, span[1]-1)] + mapped = append(mapped, []int{start, end}) + } + return mapped +} + // redactControlSplitSecrets replaces credentials whose body is interrupted by // invisible characters. It is a no-op for text containing none of them, which is -// nearly all text, so the contiguous path below keeps its behavior exactly. +// nearly all text, so the contiguous path keeps its behavior exactly. +// +// It runs over the contiguous matchers' output and stops at every replacement +// they left behind: those mark text already accounted for, and a match reaching +// across one would be reaching out of one credential and into the next. func redactControlSplitSecrets(value, replacement string) string { + if !containsSplitSeparator(value) { + return value + } + if replacement == "" { + return redactSplitRegion(value, replacement, 0) + } + regions := strings.Split(value, replacement) + if len(regions) == 1 { + return redactSplitRegion(value, replacement, 0) + } + previous := byte(0) + for i, region := range regions { + regions[i] = redactSplitRegion(region, replacement, previous) + previous = replacement[len(replacement)-1] + } + return strings.Join(regions, replacement) +} + +// redactSplitRegion is the matcher for one stretch of text with no replacement +// inside it. previous is the byte immediately before the region in the original, +// or zero at the start of the string, and settles the leading boundary for a +// match that begins at offset zero. +func redactSplitRegion(value, replacement string, previous byte) string { if !containsSplitSeparator(value) { return value } @@ -152,19 +279,22 @@ func redactControlSplitSecrets(value, replacement string) string { if len(compact) == len(value) { return value } - spans := shapeSecretSpans(compact) + spans := splitSourceSpans(value, shapeSecretSpans(compact)) if len(spans) == 0 { return value } - starts, ends := splitSeparatorIndex(value) - mapped := make([][]int, 0, len(spans)) + bounded := make([][]int, 0, len(spans)) for _, span := range spans { - if span[0] >= len(starts) || span[1] <= span[0] || span[1] > len(ends) { + before := previous + if span[0] > 0 { + before = value[span[0]-1] + } + if wordByte(before) { continue } - mapped = append(mapped, []int{starts[span[0]], ends[span[1]-1]}) + bounded = append(bounded, span) } - return spliceSpans(value, mapped, replacement) + return spliceSpans(value, bounded, replacement) } // spliceSpans replaces every named range of value with replacement, merging diff --git a/internal/redaction/control_split_test.go b/internal/redaction/control_split_test.go index be193c6f9..349309951 100644 --- a/internal/redaction/control_split_test.go +++ b/internal/redaction/control_split_test.go @@ -1,6 +1,7 @@ package redaction import ( + "runtime" "strings" "testing" ) @@ -200,3 +201,182 @@ func TestCompactionDoesNotRewriteTextItDoesNotRedact(t *testing.T) { } } } + +// The separators the tests below need by name, built from their code points so +// no escape in this file can be mangled into the character it stands for. +var ( + nulSeparator = string(rune(0x00)) + escSeparator = string(rune(0x1b)) + zwspSeparator = string(rune(0x200b)) +) + +const ( + slackKey = "xoxb-EXAMPLE-NOT-A-REAL-TOKEN-AAAAAAAAAA" + googleKey = "AIzaSyD-0123456789abcdefghijklmnopqrstuv" + jwtToken = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dBjftJeZ4CVPmB92K27uhbUJU1p1r0W1gFWFOEjXkPY" + otherJWT = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiI5ODc2NTQzMjEwIn0.QWxpY2VCb2JDYXJvbERhdmVFdmVGcmFua0dyYWNlSA" +) + +// splitInTheMiddle puts one separator inside the body of a secret. +func splitInTheMiddle(secret, separator string) string { + half := len(secret) / 2 + return secret[:half] + separator + secret[half:] +} + +// A SEPARATOR IN FRONT OF A KEY IS A DELIMITER, ONE INSIDE IT IS FILLER. +// +// #969 asks for one thing: the split form is redacted the same as the unsplit +// form. That is the whole assertion here, run over every kind of byte a key can +// follow. It catches the two ways of getting it wrong from opposite sides. Drop +// the separators and then ask the pattern for a word boundary, and a key written +// after a NUL stops redacting the moment a second separator lands inside it, +// even though the unsplit key after that same NUL redacts today. Ignore the +// boundary instead, and a key glued to the end of a word starts redacting when +// it is split, although the unsplit form never did. +func TestSplitRedactionMatchesTheUnsplitVerdictAtEveryLeadingBoundary(t *testing.T) { + leaders := map[string]string{ + "start of string": "", + "space": "before ", + "quote": `{"k":"`, + "equals": "key=", + "NUL": "prefix" + nulSeparator, + "ESC": "prefix" + escSeparator, + "zero width": "prefix" + zwspSeparator, + "letter": "prefix", + "digit": "7", + "underscore": "field_", + } + secrets := map[string]string{"aws": awsKey, "github": githubKey, "openai": openaiKey, "slack": slackKey, "google": googleKey} + for secretName, secret := range secrets { + for leaderName, leader := range leaders { + unsplit := leader + secret + split := leader + splitInTheMiddle(secret, escSeparator) + wantRedacted := !strings.Contains(RedactString(unsplit, Options{}), secret) + out := RedactString(split, Options{}) + gotRedacted := !strings.Contains(rejoin(out), secret) + if gotRedacted == wantRedacted { + continue + } + if wantRedacted { + t.Errorf("%s key after a %s leader: the unsplit form redacts and the split form does not: %q rejoins to %q", + secretName, leaderName, out, rejoin(out)) + continue + } + t.Errorf("%s key after a %s leader: the unsplit form is left alone and the split form is redacted: %q", + secretName, leaderName, out) + } + } +} + +// A MATCH MUST NOT REACH OUT OF ONE CREDENTIAL AND INTO THE NEXT. +// +// The JWT shapes end on a run of body characters with no trailing boundary, so +// a matcher reading a copy with the separators removed can start in the key in +// front and run through the header of the JWT behind it, replacing both with one +// marker and eating the delimiter between them. The contiguous pass claims whole +// credentials before this one runs, which is what keeps them apart. +func TestSplitRedactionKeepsNeighbouringCredentialsApart(t *testing.T) { + separators := map[string]string{"NUL": nulSeparator, "ESC": escSeparator, "zero width": zwspSeparator} + leaders := map[string]string{ + "jwt": jwtToken, + "github": githubKey, + "anthropic": anthropicKey, + "openai": openaiKey, + "slack": slackKey, + "google": googleKey, + "aws": awsKey, + } + for leaderName, leader := range leaders { + for sepName, separator := range separators { + value := leader + separator + otherJWT + want := RedactedSecret + separator + RedactedSecret + if got := RedactString(value, Options{}); got != want { + t.Errorf("a %s key %s a JWT: RedactString(...) = %q, want %q", leaderName, sepName, got, want) + } + } + } +} + +// ORDINARY TEXT AFTER A KEY IS NOT PART OF THE KEY. Removing the separator +// between them joins the word behind it onto the end of the key, and an +// unbounded shape then carries the replacement over text that was never secret. +func TestSplitRedactionLeavesTextBehindAKeyAlone(t *testing.T) { + for _, testCase := range []struct { + name string + value string + want string + }{ + {"word", openaiKey + nulSeparator + "ordinary prose here", RedactedSecret + nulSeparator + "ordinary prose here"}, + {"escape then word", openaiKey + escSeparator + "ordinary", RedactedSecret + escSeparator + "ordinary"}, + {"zero width then word", githubKey + zwspSeparator + "ordinary", RedactedSecret + zwspSeparator + "ordinary"}, + {"digits", awsKey + nulSeparator + "0123456789", RedactedSecret + nulSeparator + "0123456789"}, + } { + t.Run(testCase.name, func(t *testing.T) { + if got := RedactString(testCase.value, Options{}); got != testCase.want { + t.Errorf("RedactString(...) = %q, want %q", got, testCase.want) + } + }) + } +} + +// THE OFFSET TABLE IS SIZED BY THE MATCHES, NOT BY THE TEXT IT SEARCHED. +// +// RedactString is handed whole command output, verification stdout and +// notification bodies, none of which is bounded before it runs, and an index +// carrying a start and an end for every source byte costs sixteen bytes per byte +// of input on a 64-bit build. Measured against the same text with no separator +// in it, so the regex work and the output copy cancel out and what is left is +// what the split path added. +func TestSplitRedactionDoesNotIndexEverySourceByte(t *testing.T) { + const line = "plain log output line with no secret in it " + filler := strings.Repeat(line, (1<<20)/len(line)) + split := filler + " " + splitInTheMiddle(awsKey, escSeparator) + plain := filler + " " + awsKey + if got := RedactString(split, Options{}); !strings.HasSuffix(got, RedactedSecret) { + t.Fatalf("SETUP INVALID: the split key in the large input is not redacted, so nothing indexes it: %q", got[len(got)-64:]) + } + // Four times the input still fails an index of two ints per source byte, + // which needs sixteen, and clears the copy this path actually makes. + limit := int64(4 * len(split)) + overhead := redactAllocBytes(split) - redactAllocBytes(plain) + if overhead > limit { + t.Errorf("redacting %d bytes with a split key allocated %d bytes more than the same text without one, over the %d byte limit", + len(split), overhead, limit) + } +} + +// redactAllocBytes is the smallest number of bytes any of three RedactString +// calls allocated. Smallest rather than mean because a garbage collection or an +// unrelated goroutine can only ever add to the figure. +func redactAllocBytes(value string) int64 { + smallest := int64(-1) + for i := 0; i < 3; i++ { + var before, after runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&before) + out := RedactString(value, Options{}) + runtime.ReadMemStats(&after) + if len(out) == 0 { + panic("RedactString returned nothing") + } + used := int64(after.TotalAlloc - before.TotalAlloc) + if smallest < 0 || used < smallest { + smallest = used + } + } + return smallest +} + +// A REPLACEMENT ALREADY ON THE PAGE IS A BARRIER. The default marker is spelled +// with brackets, which no credential body admits, so it stops a match by +// itself. A caller is free to supply one made of ordinary word characters, and +// then the text left by the contiguous pass would read as the middle of a key +// and carry the replacement out over both delimiters and the text behind them. +func TestSplitRedactionStopsAtACallerSuppliedReplacement(t *testing.T) { + options := Options{Replacement: "REDACTED"} + value := "sk-aaaaa" + nulSeparator + awsKey + nulSeparator + "bbbbbbbbbbbbbbb" + want := "sk-aaaaa" + nulSeparator + "REDACTED" + nulSeparator + "bbbbbbbbbbbbbbb" + if got := RedactString(value, options); got != want { + t.Errorf("RedactString(...) = %q, want %q", got, want) + } +} diff --git a/internal/redaction/redaction.go b/internal/redaction/redaction.go index 836bcdcc8..211239eac 100644 --- a/internal/redaction/redaction.go +++ b/internal/redaction/redaction.go @@ -224,10 +224,6 @@ func RedactString(value string, options Options) string { } return parts[1] + parts[2] + "=" + replacement }) - // Credentials broken up by invisible characters are redacted before the - // contiguous matchers run, because those matchers cannot see them at all and - // would leave the fragments in place. No-op for text with no such character. - redacted = redactControlSplitSecrets(redacted, replacement) // openai keys first so the filter can drop kebab-case false positives // before any other pattern rewrites nearby text. redacted = openaiKeyPattern.ReplaceAllStringFunc(redacted, func(match string) string { @@ -239,6 +235,11 @@ func RedactString(value string, options Options) string { for _, pattern := range textSecretPatterns { redacted = pattern.ReplaceAllString(redacted, replacement) } + // Credentials broken up by invisible characters last, over what the + // contiguous matchers left: a whole credential is already claimed by its own + // strict match, so this pass cannot reach out of one and into the next. It is + // a no-op for text with no such character, which is nearly all text. + redacted = redactControlSplitSecrets(redacted, replacement) return redacted } From 3d6f89915aa35a862594fc3973644fb368997985 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 24 Sep 2026 13:53:26 +0530 Subject: [PATCH 4/7] fix(redaction): match split credentials with gap-tolerant shapes on the original text CodeRabbit found a bypass in the compaction approach: "xAKIAAKIAIOSFODNN7EXAMPLE" leaked while its unsplit counterpart redacts. With the separators removed first, the leftmost AWS match began at the decoy AKIA, was then rejected for the "x" in front of it, and had already consumed the start of the real key, so the real key was never considered. The ordinary-prose version is any word ending in "sk" followed by a hyphen, "ask-" or "task-", ahead of a split OpenAI key. Resuming the scan one byte after each rejection would close that but is quadratic for the greedy shapes on text like "ask-ask-ask-..." with one NUL in it. So the boundary goes back inside the pattern instead. Each shape is rewritten through regexp/syntax so a run of separators may sit between any two characters it consumes, but never before the first, and matched against the ORIGINAL text with its leading \b intact. RE2 then judges the word boundary against the real neighbour in one linear pass, and there is no compaction and no offset mapping. The separator class is generated from the same Unicode tables as splitSecretSeparator. Two further cases came out of rerunning the seeded differential against main: - The shapes are matched independently and the union of their spans replaced. Applied one after another, a split key's unbounded body ran on across a separator into a following JWT, stopped at its first dot, and left the JWT shape nothing to match, exposing its payload and signature. - A second pass, ungated, catches a credential glued onto a split one, which gains its word boundary only once the first is replaced. It is often whole itself, so its region can hold no separator by then. On 6,000 control-free inputs the output is byte-for-byte main's. On 6,000 with invisible characters, 147 outputs still hold a known credential once those are removed, against 2112 on main; 140 of the 147 leak on main too when the input is unsplit, and the remaining 7 are the one limit now written into the header comment: a split key whose first fragment is a complete credential on its own, with another credential glued onto its tail. --- internal/redaction/control_split.go | 403 +++++++++++++---------- internal/redaction/control_split_test.go | 129 ++++++++ 2 files changed, 356 insertions(+), 176 deletions(-) diff --git a/internal/redaction/control_split.go b/internal/redaction/control_split.go index b817ad6c1..db3efc519 100644 --- a/internal/redaction/control_split.go +++ b/internal/redaction/control_split.go @@ -1,7 +1,9 @@ package redaction import ( + "fmt" "regexp" + "regexp/syntax" "sort" "strings" "unicode" @@ -21,36 +23,54 @@ import ( // Cc and Cf character behaves the same way, which is why this is written against // the classes rather than against the two bytes named there. // -// The rule is normalize-then-match, applied so that the ORIGINAL bytes are what -// gets replaced: matching on a compacted copy and then redacting that copy would -// hand back a string missing the separators, which is a different string from -// the one the caller passed in. So the spans are mapped back and the original -// slice, separators included, is what the replacement covers. +// HOW: each shape is rewritten so that a run of separators may sit between any +// two characters it consumes, and matched against the ORIGINAL text. Nothing is +// removed before matching, so there are no offsets to map back, and the leading +// \b of every shape is judged by the regexp engine against the real neighbour: +// a key written after a NUL starts at a boundary, exactly as the unsplit key +// does, and a key glued to the end of a word does not. // -// THE SAME CHARACTER IS FILLER INSIDE A CREDENTIAL AND A DELIMITER OUTSIDE ONE, -// and the difference cannot be read off the compacted text, so it is taken from -// the two places that do know it: +// An earlier version removed the separators first and matched the compacted +// copy. That lost the difference between a separator inside a credential and one +// next to it, and the fixes for that (check the boundary against the source +// afterwards, map offsets back) left a hole: in "xAKIAAKIAIOSFODNN7..." +// the leftmost compacted match began at the decoy AKIA, was then rejected for +// the "x" in front of it, and had already consumed the start of the real key, so +// the real key was never considered. Resuming the scan one byte after each +// rejection would close that, but it is quadratic for the greedy shapes on text +// like "ask-ask-ask-..." with a single NUL in it. Keeping the boundary inside +// the pattern lets RE2 do it in one linear pass. // -// - This pass runs AFTER the contiguous matchers, over their output, and never -// across a replacement. A credential that is whole gets claimed by its own -// strict match, so two whole credentials with a separator between them stay -// two replacements with the separator still between them, rather than one -// match reaching out of the first and into the second. -// - The leading word boundary is checked against the SOURCE, not against the -// joined text. A separator in front of a key is a boundary, and a whole key -// written after a NUL already redacts, so removing that NUL must not turn -// the key into the tail of the word in front of it. The patterns below drop -// their leading boundary, and the byte before the match's ORIGINAL start is -// what has to be a non-word byte instead. So "prefixAKIA......" -// redacts, because unsplit "prefixAKIA..." does, and "xAKIA......" -// does not, because unsplit "xAKIA..." does not either. +// This pass runs AFTER the contiguous matchers, over their output, and never +// across a replacement. A credential that is whole is claimed by its own strict +// match, so two whole credentials with a separator between them stay two +// replacements with the separator still between them. The shapes run one at a +// time, like the contiguous pass, so a replacement can give the next shape the +// boundary it would otherwise lack. // -// WHAT THIS DELIBERATELY DOES NOT STRIP: tab, newline and carriage return. -// Those three are real text structure rather than invisible filler, so a -// credential broken across a line boundary is still two visible fragments here -// and is not covered. Stripping them would also let a match span a line break -// and collapse unrelated lines into one replacement. A credential split that way -// is visible to whoever reads it; the ones handled here are not. +// A match never starts or ends on a separator, so the separators on either side +// of a credential stay where they were: "AKIA..." becomes +// "[REDACTED]", as it does for the unsplit key. +// +// ONE KNOWN LIMIT, from running the contiguous matchers first. When the part of +// a split key BEFORE its first separator is already a complete credential on +// its own, the contiguous matcher claims that part and stops at the separator, +// so what follows is judged by itself. Usually that is only the tail of the key, +// which is not a credential alone. It matters only when another credential is +// glued straight onto that tail with no boundary between them: the unsplit key +// would have run on and swallowed it, and here it stays. Extending the +// contiguous match across the separator instead would bring back what running +// first exists to prevent, prose after a whole key being eaten and a following +// JWT losing its header. +// +// WHAT THIS DELIBERATELY DOES NOT TREAT AS A SEPARATOR: tab, newline and +// carriage return. Those three are real text structure rather than invisible +// filler, so a credential broken across a line boundary is still two visible +// fragments here and is not covered. Treating them as gaps would also let a +// match span a line break and collapse unrelated lines into one replacement. A +// credential split that way is visible to whoever reads it; the ones handled +// here are not. Invalid UTF-8 is out for the same reason: the engine reads an +// invalid byte as U+FFFD, which is drawn. // splitSecretSeparator reports a character that can sit inside a credential // without being seen. Cc covers the C0 and C1 controls (NUL, ESC, DEL, NEL, CSI) @@ -64,11 +84,11 @@ func splitSecretSeparator(r rune) bool { return unicode.Is(unicode.Cc, r) || unicode.Is(unicode.Cf, r) } -// containsSplitSeparator is the cheap gate in front of the compaction, so text -// with nothing invisible in it, which is nearly all text, costs one scan and no -// allocation. ASCII is settled from the byte alone; only a byte that could begin -// a multi-byte rune needs decoding, and the C1 and Cf blocks all start with a -// lead byte at or above 0xC2. +// containsSplitSeparator is the cheap gate in front of the gap-tolerant pass, so +// text with nothing invisible in it, which is nearly all text, costs one scan and +// no allocation. ASCII is settled from the byte alone; only a byte that could +// begin a multi-byte rune needs decoding, and the C1 and Cf blocks all start +// with a lead byte at or above 0xC2. func containsSplitSeparator(value string) bool { for i := 0; i < len(value); i++ { b := value[i] @@ -90,13 +110,9 @@ func containsSplitSeparator(value string) bool { return false } -// compactSplitSeparators returns value with every splitSecretSeparator removed. -// -// Decoded explicitly rather than with range, because an invalid byte must be -// measured as the one byte it occupies. Ranging reports it as U+FFFD, whose -// encoded width is three, and the offsets this feeds have to stay in the -// source's own bytes. -func compactSplitSeparators(value string) string { +// stripSplitSeparators returns value with every splitSecretSeparator removed. +// Used only on a matched credential, to judge it by its visible characters. +func stripSplitSeparators(value string) string { var builder strings.Builder builder.Grow(len(value)) for offset := 0; offset < len(value); { @@ -109,61 +125,132 @@ func compactSplitSeparators(value string) string { return builder.String() } -// leadingWordBoundary is the token every shape matcher opens with. The split -// matchers drop it and check the source byte instead; see the header comment. -const leadingWordBoundary = `\b` +// splitSeparatorClass is splitSecretSeparator as a regexp character class, +// generated from the same Unicode tables and the same three exclusions so the +// class and the predicate cannot disagree. +var splitSeparatorClass = func() string { + var class strings.Builder + class.WriteByte('[') + emit := func(lo, hi rune) { + fmt.Fprintf(&class, `\x{%x}-\x{%x}`, lo, hi) + } + for _, table := range []*unicode.RangeTable{unicode.Cc, unicode.Cf} { + visit := func(lo, hi, stride uint32) { + for r := lo; r <= hi; r += stride { + start := rune(r) + if !splitSecretSeparator(start) { + continue + } + end := start + if stride == 1 { + for next := rune(r) + 1; uint32(next) <= hi && splitSecretSeparator(next); next++ { + end = next + } + r = uint32(end) + } + emit(start, end) + } + } + for _, r := range table.R16 { + visit(uint32(r.Lo), uint32(r.Hi), uint32(r.Stride)) + } + for _, r := range table.R32 { + visit(r.Lo, r.Hi, r.Stride) + } + } + class.WriteByte(']') + return class.String() +}() -// splitOpenAIPattern and splitTextPatterns are the shape matchers with their -// leading word boundary removed, derived from the originals rather than written -// out a second time so the two lists cannot drift apart. +// splitOpenAIPattern and splitTextPatterns are the shape matchers with a run of +// separators allowed between every two characters they consume, derived from +// the originals rather than written out a second time so the two lists cannot +// drift apart. var ( - splitOpenAIPattern = withoutLeadingBoundary(openaiKeyPattern) - splitTextPatterns = withoutLeadingBoundaries(textSecretPatterns) + splitOpenAIPattern = gapTolerant(openaiKeyPattern) + splitTextPatterns = gapTolerantAll(textSecretPatterns) ) -func withoutLeadingBoundary(pattern *regexp.Regexp) *regexp.Regexp { - source := pattern.String() - relaxed := strings.TrimPrefix(source, leadingWordBoundary) - if relaxed == source { - return pattern - } - return regexp.MustCompile(relaxed) -} - -func withoutLeadingBoundaries(patterns []*regexp.Regexp) []*regexp.Regexp { - relaxed := make([]*regexp.Regexp, 0, len(patterns)) +func gapTolerantAll(patterns []*regexp.Regexp) []*regexp.Regexp { + tolerant := make([]*regexp.Regexp, 0, len(patterns)) for _, pattern := range patterns { - relaxed = append(relaxed, withoutLeadingBoundary(pattern)) + tolerant = append(tolerant, gapTolerant(pattern)) } - return relaxed + return tolerant } -// wordByte is the class Go's regexp word boundary is defined over: ASCII -// letters, digits and underscore, and nothing else. A multi-byte lead or -// continuation byte falls outside it, which is what the boundary already -// assumes. -func wordByte(b byte) bool { - return b == '_' || - ('0' <= b && b <= '9') || - ('A' <= b && b <= 'Z') || - ('a' <= b && b <= 'z') +// gapTolerant rewrites pattern so that a run of separators may appear before +// every character it consumes except the first. Zero-width assertions, the +// leading \b in particular, are left exactly where they were. +func gapTolerant(pattern *regexp.Regexp) *regexp.Regexp { + tree, err := syntax.Parse(pattern.String(), syntax.Perl) + if err != nil { + panic(fmt.Sprintf("redaction: parse %q: %v", pattern.String(), err)) + } + separator, err := syntax.Parse(splitSeparatorClass, syntax.Perl) + if err != nil { + panic(fmt.Sprintf("redaction: parse separator class: %v", err)) + } + consumed := false + return regexp.MustCompile(withGaps(tree, separator, &consumed).String()) } -// shapeSecretSpans returns the byte ranges of value claimed by the -// boundary-relaxed credential shapes, applying the same openai filter -// RedactString uses so the two agree on what a key is. -func shapeSecretSpans(value string) [][]int { - var spans [][]int - for _, span := range splitOpenAIPattern.FindAllStringIndex(value, -1) { - if !openAIShapeIsSecret(value[span[0]:span[1]]) { - continue +// withGaps inserts the separator run before each consuming atom. consumed +// tracks whether anything has been consumed yet on this path, because the first +// character of a match must be a real one: a leading gap would let a match start +// on a separator and swallow it. +func withGaps(node, separator *syntax.Regexp, consumed *bool) *syntax.Regexp { + switch node.Op { + case syntax.OpLiteral: + parts := make([]*syntax.Regexp, 0, len(node.Rune)) + for _, r := range node.Rune { + atom := &syntax.Regexp{Op: syntax.OpLiteral, Rune: []rune{r}, Flags: node.Flags} + parts = append(parts, gapBefore(atom, separator, consumed)) + } + if len(parts) == 1 { + return parts[0] + } + return &syntax.Regexp{Op: syntax.OpConcat, Sub: parts} + case syntax.OpCharClass, syntax.OpAnyChar, syntax.OpAnyCharNotNL: + return gapBefore(node, separator, consumed) + case syntax.OpConcat, syntax.OpCapture: + for i := range node.Sub { + node.Sub[i] = withGaps(node.Sub[i], separator, consumed) + } + return node + case syntax.OpAlternate: + start, after := *consumed, *consumed + for i := range node.Sub { + branch := start + node.Sub[i] = withGaps(node.Sub[i], separator, &branch) + after = after || branch + } + *consumed = after + return node + case syntax.OpStar, syntax.OpPlus, syntax.OpQuest, syntax.OpRepeat: + // Every shape opens with a literal prefix, so a repetition is never the + // first thing a match consumes. If one ever is, its first iteration would + // need no gap and the rest would; fail loudly rather than guess. + if !*consumed { + panic("redaction: gap-tolerant rewrite reached a repetition before any literal") } - spans = append(spans, span) + node.Sub[0] = withGaps(node.Sub[0], separator, consumed) + return node + case syntax.OpWordBoundary, syntax.OpNoWordBoundary, syntax.OpBeginLine, syntax.OpEndLine, + syntax.OpBeginText, syntax.OpEndText, syntax.OpEmptyMatch: + return node + default: + panic(fmt.Sprintf("redaction: gap-tolerant rewrite does not handle %v", node.Op)) } - for _, pattern := range splitTextPatterns { - spans = append(spans, pattern.FindAllStringIndex(value, -1)...) +} + +func gapBefore(atom, separator *syntax.Regexp, consumed *bool) *syntax.Regexp { + if !*consumed { + *consumed = true + return atom } - return spans + gap := &syntax.Regexp{Op: syntax.OpStar, Sub: []*syntax.Regexp{separator}} + return &syntax.Regexp{Op: syntax.OpConcat, Sub: []*syntax.Regexp{gap, atom}} } // openAIShapeIsSecret is the digit/kebab-case filter RedactString applies to an @@ -175,72 +262,6 @@ func openAIShapeIsSecret(match string) bool { return !strings.Contains(strings.TrimPrefix(match, "sk-"), "-") } -// splitSourceSpans maps match ranges found in compactSplitSeparators(value) -// back onto value, returning the original slice each one stands for. -// -// Only the endpoints of actual matches are resolved. An index over every source -// byte would cost at least sixteen bytes per byte of input on a 64-bit build, -// and RedactString is handed whole command output and notification bodies, so -// that table would be sized by the text rather than by the number of credentials -// in it. The walk below is a single pass whose memory is two ints per match -// endpoint. -func splitSourceSpans(value string, spans [][]int) [][]int { - if len(spans) == 0 { - return nil - } - wanted := make([]int, 0, 2*len(spans)) - for _, span := range spans { - if span[1] <= span[0] { - continue - } - wanted = append(wanted, span[0], span[1]-1) - } - if len(wanted) == 0 { - return nil - } - sort.Ints(wanted) - unique := wanted[:1] - for _, offset := range wanted[1:] { - if offset != unique[len(unique)-1] { - unique = append(unique, offset) - } - } - wanted = unique - - starts := make([]int, len(wanted)) - ends := make([]int, len(wanted)) - next := 0 - compact := 0 - for offset := 0; offset < len(value) && next < len(wanted); { - r, width := utf8.DecodeRuneInString(value[offset:]) - if splitSecretSeparator(r) { - offset += width - continue - } - for next < len(wanted) && wanted[next] < compact+width { - starts[next] = offset - ends[next] = offset + width - next++ - } - compact += width - offset += width - } - if next < len(wanted) { - return nil - } - - mapped := make([][]int, 0, len(spans)) - for _, span := range spans { - if span[1] <= span[0] { - continue - } - start := starts[sort.SearchInts(wanted, span[0])] - end := ends[sort.SearchInts(wanted, span[1]-1)] - mapped = append(mapped, []int{start, end}) - } - return mapped -} - // redactControlSplitSecrets replaces credentials whose body is interrupted by // invisible characters. It is a no-op for text containing none of them, which is // nearly all text, so the contiguous path keeps its behavior exactly. @@ -248,57 +269,84 @@ func splitSourceSpans(value string, spans [][]int) [][]int { // It runs over the contiguous matchers' output and stops at every replacement // they left behind: those mark text already accounted for, and a match reaching // across one would be reaching out of one credential and into the next. +// +// It runs at most twice. The second pass exists for a credential glued straight +// onto the end of another split one: it has no word boundary of its own until +// the first is replaced, and the replacement then supplies one, the same way the +// contiguous shapes cascade into each other. Two passes and no more keeps the +// cost linear; a longer chain of glued split credentials is left as the +// contiguous matchers leave a chain of glued whole ones. func redactControlSplitSecrets(value, replacement string) string { if !containsSplitSeparator(value) { return value } + once := redactSplitRegions(value, replacement, true) + if once == value { + return value + } + // Ungated: the credential the second pass exists for is often contiguous + // itself (a whole JWT glued onto a split AWS key), so its region can hold no + // separator at all once the first pass has cut the text around it. + return redactSplitRegions(once, replacement, false) +} + +func redactSplitRegions(value, replacement string, gated bool) string { if replacement == "" { - return redactSplitRegion(value, replacement, 0) + return redactSplitRegion(value, replacement, 0, gated) } regions := strings.Split(value, replacement) if len(regions) == 1 { - return redactSplitRegion(value, replacement, 0) + return redactSplitRegion(value, replacement, 0, gated) } previous := byte(0) for i, region := range regions { - regions[i] = redactSplitRegion(region, replacement, previous) + regions[i] = redactSplitRegion(region, replacement, previous, gated) previous = replacement[len(replacement)-1] } return strings.Join(regions, replacement) } -// redactSplitRegion is the matcher for one stretch of text with no replacement -// inside it. previous is the byte immediately before the region in the original, -// or zero at the start of the string, and settles the leading boundary for a -// match that begins at offset zero. -func redactSplitRegion(value, replacement string, previous byte) string { - if !containsSplitSeparator(value) { +// redactSplitRegion runs every gap-tolerant shape over one stretch of text with +// no replacement inside it and replaces the UNION of what they found. +// +// Independently, not one after another. An unbounded body with gaps allowed will +// run on across a separator into whatever follows, and when that is the start of +// another credential it takes that credential's prefix with it: in +// "sk-ant-…buildeyJ….eyJ….sig" the Anthropic shape stops only at the +// JWT's first dot. Applied in sequence, the JWT shape then finds its header gone +// and the payload and signature are left in the clear. Matched against the same +// text, the JWT shape still finds the whole token, and the union covers both. +// +// previous is the byte immediately before the region in the original, or zero at +// the start of the string. It is lent to the engine in front of the region so a +// leading \b is judged against the real neighbour, and a match is never allowed +// to begin on it. +func redactSplitRegion(value, replacement string, previous byte, gated bool) string { + if gated && !containsSplitSeparator(value) { return value } - compact := compactSplitSeparators(value) - if len(compact) == len(value) { - return value + text, lead := value, 0 + if previous != 0 { + text, lead = string([]byte{previous})+value, 1 } - spans := splitSourceSpans(value, shapeSecretSpans(compact)) - if len(spans) == 0 { - return value - } - bounded := make([][]int, 0, len(spans)) - for _, span := range spans { - before := previous - if span[0] > 0 { - before = value[span[0]-1] + var spans [][]int + for _, span := range splitOpenAIPattern.FindAllStringIndex(text, -1) { + if span[0] >= lead && openAIShapeIsSecret(stripSplitSeparators(text[span[0]:span[1]])) { + spans = append(spans, span) } - if wordByte(before) { - continue + } + for _, pattern := range splitTextPatterns { + for _, span := range pattern.FindAllStringIndex(text, -1) { + if span[0] >= lead { + spans = append(spans, span) + } } - bounded = append(bounded, span) } - return spliceSpans(value, bounded, replacement) + return spliceSpans(text, spans, replacement)[lead:] } // spliceSpans replaces every named range of value with replacement, merging -// ranges that overlap or touch so one credential cannot be replaced twice. +// ranges that overlap so one stretch of text is never replaced twice. func spliceSpans(value string, spans [][]int, replacement string) string { if len(spans) == 0 { return value @@ -314,10 +362,13 @@ func spliceSpans(value string, spans [][]int, replacement string) string { written := 0 for _, span := range spans { if span[0] < written { - if span[1] <= written { - continue + // Overlaps the stretch just replaced: extend it rather than emit a + // second marker, which would claim two credentials where there was one + // stretch of text. + if span[1] > written { + written = span[1] } - span[0] = written + continue } builder.WriteString(value[written:span[0]]) builder.WriteString(replacement) diff --git a/internal/redaction/control_split_test.go b/internal/redaction/control_split_test.go index 349309951..89325de86 100644 --- a/internal/redaction/control_split_test.go +++ b/internal/redaction/control_split_test.go @@ -1,9 +1,12 @@ package redaction import ( + "fmt" + "regexp" "runtime" "strings" "testing" + "unicode" ) const ( @@ -380,3 +383,129 @@ func TestSplitRedactionStopsAtACallerSuppliedReplacement(t *testing.T) { t.Errorf("RedactString(...) = %q, want %q", got, want) } } + +// A REJECTED MATCH MUST NOT SHADOW THE REAL KEY BEHIND IT. Raised by CodeRabbit +// on #1067. With the separators removed before matching, the leftmost AWS match +// in "xAKIAAKIAIOSFODNN7EXAMPLE" began at the decoy AKIA, was rejected +// for the "x" in front of it, and had already consumed the start of the real key, +// so the real key was never tried. "ask-" in front of a split OpenAI key does the +// same through "sk-", which is the ordinary-prose version of it. Each input is +// checked against its unsplit counterpart, which main already redacts. +func TestSplitRedactionDoesNotLetARejectedMatchShadowTheRealKey(t *testing.T) { + soh := string(rune(0x01)) + for _, testCase := range []struct { + name string + split string + unsplit string + }{ + {"decoy AKIA before a split key", "xAKIA" + nulSeparator + "AKIAIOSFODNN7E" + soh + "XAMPLE", "xAKIA" + nulSeparator + awsKey}, + {"ask- before a split OpenAI key", "ask-" + nulSeparator + splitInTheMiddle(openaiKey, soh), "ask-" + nulSeparator + openaiKey}, + {"task- before a split OpenAI key", "task-" + escSeparator + splitInTheMiddle(openaiKey, zwspSeparator), "task-" + escSeparator + openaiKey}, + } { + t.Run(testCase.name, func(t *testing.T) { + want := RedactString(testCase.unsplit, Options{}) + if strings.Contains(want, awsKey) || strings.Contains(want, openaiKey) { + t.Fatalf("SETUP INVALID: main does not redact the unsplit form either: %q", want) + } + if got := RedactString(testCase.split, Options{}); got != want { + t.Errorf("RedactString(split) = %q, want the unsplit form's %q", got, want) + } + }) + } +} + +// AN UNBOUNDED BODY MUST NOT TAKE A NEIGHBOUR'S PREFIX WITH IT. With gaps allowed +// between body characters, a split key's body runs on across a separator into +// whatever follows. When that is a JWT, it stops only at the JWT's first dot, and +// if the shapes were applied one after another the JWT shape would then find its +// header gone and leave the payload and signature in the clear. The shapes are +// matched against the same text and their union replaced, so it cannot. +func TestSplitRedactionKeepsANeighbourWhosePrefixASplitBodyWouldSwallow(t *testing.T) { + cut := len(jwtToken) / 2 + splitJWT := jwtToken[:cut] + nulSeparator + jwtToken[cut:] + for _, testCase := range []struct { + name string + value string + }{ + {"split Anthropic key, a word, then a split JWT", splitInTheMiddle(anthropicKey, zwspSeparator) + escSeparator + "build" + escSeparator + splitJWT}, + {"split GitHub key, then a split JWT", splitInTheMiddle(githubKey, nulSeparator) + escSeparator + splitJWT}, + } { + t.Run(testCase.name, func(t *testing.T) { + got := rejoin(RedactString(testCase.value, Options{})) + for _, piece := range strings.Split(jwtToken, ".") { + if strings.Contains(got, piece) { + t.Errorf("part of the JWT survives: %q in %q", piece, got) + } + } + }) + } +} + +// A CREDENTIAL GLUED ONTO A SPLIT ONE GETS ITS BOUNDARY FROM THE REPLACEMENT. +// Contiguous shapes cascade: once an AWS key is replaced, a JWT written straight +// after it has the "]" in front of it and matches. A split key is replaced by +// the second pass, so the JWT behind it needs a pass of its own after that one, +// and the JWT is often whole, which means its region may hold no separator at +// all by then. The second case adds a whole key elsewhere so that the strict +// pass has already cut the text into regions. +func TestSplitRedactionCascadesIntoACredentialGluedOntoASplitOne(t *testing.T) { + csi := string(rune(0x9b)) + for _, testCase := range []struct { + name string + value string + }{ + {"whole JWT glued onto a split AWS key", splitInTheMiddle(awsKey, escSeparator) + jwtToken}, + {"same, with a whole key elsewhere in the text", "user?q=" + "ASIAIOSFODNN7EXA" + csi + "MPLE" + jwtToken + "'" + awsKey + string(rune(0x7f)) + "ok"}, + } { + t.Run(testCase.name, func(t *testing.T) { + got := rejoin(RedactString(testCase.value, Options{})) + for _, piece := range strings.Split(jwtToken, ".") { + if strings.Contains(got, piece) { + t.Errorf("part of the JWT survives: %q in %q", piece, got) + } + } + }) + } +} + +// THE CLASS AND THE PREDICATE ARE ONE FACT. splitSeparatorClass is generated +// from the same tables as splitSecretSeparator; this walks every code point and +// fails on the first one they disagree about. +func TestSplitSeparatorClassAgreesWithThePredicate(t *testing.T) { + class := regexp.MustCompile("^" + splitSeparatorClass + "$") + for r := rune(0); r <= unicode.MaxRune; r++ { + if r >= 0xd800 && r <= 0xdfff { + continue // surrogates are not valid in a Go string + } + if got, want := class.MatchString(string(r)), splitSecretSeparator(r); got != want { + t.Fatalf("U+%04X: class matches = %v, predicate says %v", r, got, want) + } + } +} + +// ON TEXT WITH NO SEPARATOR, EVERY GAP-TOLERANT SHAPE IS ITS ORIGINAL. The +// rewrite only adds optional gaps, so a contiguous key has to produce exactly the +// same match; anything else means the rewrite changed what a shape is. +func TestGapTolerantShapesMatchContiguousKeysLikeTheOriginals(t *testing.T) { + inputs := []string{ + "key=" + awsKey + " and " + githubKey, + "x" + awsKey, + anthropicKey + "." + openaiKey, + jwtToken + " then " + slackKey + ", " + googleKey, + "sk-this-is-kebab-case-prose-not-a-key", + "github_pat_11ABCDEFG0123456789_abcdefghijklmnopqrstuvwxyzABCDEFGH/glpat-abcdefghij0123456789", + } + originals := append([]*regexp.Regexp{openaiKeyPattern}, textSecretPatterns...) + tolerant := append([]*regexp.Regexp{splitOpenAIPattern}, splitTextPatterns...) + if len(originals) != len(tolerant) { + t.Fatalf("SETUP INVALID: %d original shapes and %d gap-tolerant ones", len(originals), len(tolerant)) + } + for i := range originals { + for _, input := range inputs { + want := fmt.Sprint(originals[i].FindAllStringIndex(input, -1)) + if got := fmt.Sprint(tolerant[i].FindAllStringIndex(input, -1)); got != want { + t.Errorf("shape %s on %q: gap-tolerant matched %s, original %s", originals[i], input, got, want) + } + } + } +} From 8d2fff4c565dd3baddd8352bad7953cfdf62ac9e Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 24 Sep 2026 15:45:40 +0530 Subject: [PATCH 5/7] fix(redaction): keep two split credentials of one shape apart Matches of one pattern never overlap, so when a split credential's body ran on through the separator after it, a following credential of the same shape could not start a match of its own. A JWT body stops at the next token's first dot and a classic GitHub body at its underscore, so two split JWTs side by side left the second one's payload and signature in the clear. A match the OpenAI filter rejects spans what it ran into as well, so a split key after kebab-case prose was rejected with the prose and printed. When a match runs through a separator run into a position where the same shape matches again and reaches at least as far, that credential is taken on its own, and the first is cut at the separators when it is a complete match by itself. Each piece is filtered on its own. Candidates are tried latest first and only where the shape's literal prefix follows the separators, at most eight per match, so the pass stays linear. The header comment now says plainly that the rest of a key whose front part is complete alone stays visible, and drops a stale sentence from before the shapes were matched independently. --- internal/redaction/control_split.go | 173 ++++++++++++++++++++--- internal/redaction/control_split_test.go | 77 ++++++++++ 2 files changed, 231 insertions(+), 19 deletions(-) diff --git a/internal/redaction/control_split.go b/internal/redaction/control_split.go index db3efc519..62967f1ba 100644 --- a/internal/redaction/control_split.go +++ b/internal/redaction/control_split.go @@ -44,9 +44,8 @@ import ( // This pass runs AFTER the contiguous matchers, over their output, and never // across a replacement. A credential that is whole is claimed by its own strict // match, so two whole credentials with a separator between them stay two -// replacements with the separator still between them. The shapes run one at a -// time, like the contiguous pass, so a replacement can give the next shape the -// boundary it would otherwise lack. +// replacements with the separator still between them. Two split credentials do +// too: see spansFor. // // A match never starts or ends on a separator, so the separators on either side // of a credential stay where they were: "AKIA..." becomes @@ -55,13 +54,14 @@ import ( // ONE KNOWN LIMIT, from running the contiguous matchers first. When the part of // a split key BEFORE its first separator is already a complete credential on // its own, the contiguous matcher claims that part and stops at the separator, -// so what follows is judged by itself. Usually that is only the tail of the key, -// which is not a credential alone. It matters only when another credential is -// glued straight onto that tail with no boundary between them: the unsplit key -// would have run on and swallowed it, and here it stays. Extending the -// contiguous match across the separator instead would bring back what running -// first exists to prevent, prose after a whole key being eaten and a following -// JWT losing its header. +// and what follows is judged by itself. That is the rest of the key, which is no +// credential alone, so it stays visible: a fragment of the key, never the whole +// of it, because the part in front is replaced. For the same reason a credential +// glued straight onto that rest is left, where the unsplit key would have run on +// and swallowed it. Carrying the contiguous match across the separator instead +// would bring back what running first exists to prevent, since nothing tells the +// rest of a key from prose written after a whole one: that prose would be eaten, +// and a following JWT would lose its header. // // WHAT THIS DELIBERATELY DOES NOT TREAT AS A SEPARATOR: tab, newline and // carriage return. Those three are real text structure rather than invisible @@ -171,6 +171,146 @@ var ( splitTextPatterns = gapTolerantAll(textSecretPatterns) ) +// splitShape is one credential shape as the split pass uses it. +type splitShape struct { + // free finds the shape anywhere, leftmost first. anchored matches it only + // where the text begins, and whole only when it spans the entire text. + free, anchored, whole *regexp.Regexp + // prefix is the literal every match of the shape begins with. + prefix string + // accept is the test a match has to pass to be a credential, or nil when + // every match is one. + accept func(match string) bool +} + +var splitShapes = func() []splitShape { + shapes := []splitShape{newSplitShape(openaiKeyPattern, splitOpenAIPattern, func(match string) bool { + return openAIShapeIsSecret(stripSplitSeparators(match)) + })} + for i, pattern := range textSecretPatterns { + shapes = append(shapes, newSplitShape(pattern, splitTextPatterns[i], nil)) + } + return shapes +}() + +func newSplitShape(original, free *regexp.Regexp, accept func(string) bool) splitShape { + return splitShape{ + free: free, + anchored: regexp.MustCompile(`\A(?:` + free.String() + `)`), + whole: regexp.MustCompile(`\A(?:` + free.String() + `)\z`), + prefix: leadingLiteral(original), + accept: accept, + } +} + +// leadingLiteral is the literal every match of pattern begins with, read past a +// leading \b, or "" when the pattern does not open with one. +func leadingLiteral(pattern *regexp.Regexp) string { + tree, err := syntax.Parse(pattern.String(), syntax.Perl) + if err != nil { + panic(fmt.Sprintf("redaction: parse %q: %v", pattern.String(), err)) + } + nodes := []*syntax.Regexp{tree} + if tree.Op == syntax.OpConcat { + nodes = tree.Sub + } + for _, node := range nodes { + if node.Op == syntax.OpWordBoundary { + continue + } + if node.Op == syntax.OpLiteral { + return string(node.Rune) + } + return "" + } + return "" +} + +// maxRunOnCandidates bounds the anchored matches tried inside one match, so a +// match full of lookalike starts still costs a fixed number of them. +const maxRunOnCandidates = 8 + +// spansFor turns one match of the shape into the stretches to replace. +// +// A MATCH CAN RUN ON INTO THE NEXT CREDENTIAL OF ITS OWN SHAPE. Gaps are allowed +// inside a match, and a separator between two credentials looks exactly like one +// inside a credential, so a greedy body runs through it into whatever follows. A +// credential of another shape is still found, because every shape is matched on +// its own and the union is replaced. One of the SAME shape is not: matches of one +// pattern never overlap, so the second credential cannot start inside the first +// one's match. Most bodies swallow it whole, so nothing leaks, but a JWT stops at +// the next token's first dot, and two split JWTs side by side left the second +// one's payload and signature in the clear. And a match the filter rejects hides +// whatever it ran into, so kebab-case prose in front of a split key took the key +// down with it. Reported by @jatmn. +// +// So when a match runs through a run of separators into a position where the +// same shape matches again and reaches at least as far, that credential is taken +// on its own, and the first is cut at the separators if it is a complete match by +// itself. The separators between the two then stay outside both replacements, as +// they do between two whole credentials, and each piece is filtered on its own. +// When the first is not complete alone, it keeps the whole of its match and the +// union joins the two into one replacement. +func (shape splitShape) spansFor(text string, start, end int) [][]int { + var spans [][]int + for { + next, gap, nextEnd := shape.runOn(text, start, end) + if next < 0 { + return shape.keep(spans, text, start, end) + } + if shape.whole.MatchString(text[start:gap]) { + spans = shape.keep(spans, text, start, gap) + } else { + spans = shape.keep(spans, text, start, end) + } + start, end = next, nextEnd + } +} + +func (shape splitShape) keep(spans [][]int, text string, start, end int) [][]int { + if shape.accept != nil && !shape.accept(text[start:end]) { + return spans + } + return append(spans, []int{start, end}) +} + +// runOn looks inside the match [start, end) for where it ran through a run of +// separators into another credential of the same shape. It returns where that +// credential begins, where the separator run in front of it begins, and where the +// credential's own match ends, or next = -1 when there is none. +// +// Latest first, because a match reaches into the next credential only as far as +// its own pattern allows. Only where the text after the separators opens with the +// shape's literal prefix, which is where a match of the shape can begin; the +// separator in front supplies the word boundary, so matching from there judges it +// the same as the whole text would. +func (shape splitShape) runOn(text string, start, end int) (next, gap, nextEnd int) { + tried := 0 + for at := end; at > start && tried < maxRunOnCandidates; { + r, size := utf8.DecodeLastRuneInString(text[start:at]) + if !splitSecretSeparator(r) { + at -= size + continue + } + candidate := at + for at > start { + r, size = utf8.DecodeLastRuneInString(text[start:at]) + if !splitSecretSeparator(r) { + break + } + at -= size + } + if !strings.HasPrefix(text[candidate:], shape.prefix) { + continue + } + tried++ + if match := shape.anchored.FindStringIndex(text[candidate:]); match != nil && candidate+match[1] >= end { + return candidate, at, candidate + match[1] + } + } + return -1, 0, 0 +} + func gapTolerantAll(patterns []*regexp.Regexp) []*regexp.Regexp { tolerant := make([]*regexp.Regexp, 0, len(patterns)) for _, pattern := range patterns { @@ -330,15 +470,10 @@ func redactSplitRegion(value, replacement string, previous byte, gated bool) str text, lead = string([]byte{previous})+value, 1 } var spans [][]int - for _, span := range splitOpenAIPattern.FindAllStringIndex(text, -1) { - if span[0] >= lead && openAIShapeIsSecret(stripSplitSeparators(text[span[0]:span[1]])) { - spans = append(spans, span) - } - } - for _, pattern := range splitTextPatterns { - for _, span := range pattern.FindAllStringIndex(text, -1) { - if span[0] >= lead { - spans = append(spans, span) + for _, shape := range splitShapes { + for _, match := range shape.free.FindAllStringIndex(text, -1) { + if match[0] >= lead { + spans = append(spans, shape.spansFor(text, match[0], match[1])...) } } } diff --git a/internal/redaction/control_split_test.go b/internal/redaction/control_split_test.go index 89325de86..7aacedc29 100644 --- a/internal/redaction/control_split_test.go +++ b/internal/redaction/control_split_test.go @@ -300,6 +300,83 @@ func TestSplitRedactionKeepsNeighbouringCredentialsApart(t *testing.T) { } } +// splitThroughout puts a separator a quarter of the way into every dot-separated +// segment of a token. Early, so the part in front is too short to be a +// credential by itself and the strict pass cannot claim it; a split whose front +// part is complete alone is the documented limit, not what these tests are about. +func splitThroughout(secret, separator string) string { + parts := strings.Split(secret, ".") + for i, part := range parts { + quarter := len(part) / 4 + parts[i] = part[:quarter] + separator + part[quarter:] + } + return strings.Join(parts, ".") +} + +// TWO SPLIT CREDENTIALS OF ONE SHAPE ARE TWO CREDENTIALS. +// +// Matches of one pattern never overlap, so when the first one's body runs on +// through the separator between them, the second cannot start a match of its +// own. Whole neighbours are safe because the strict pass claims them first; +// these are both split. A JWT body stops at the next token's first dot and a +// classic GitHub body at the next token's underscore, which left the rest of the +// second credential in the clear, and the bodies that swallow the next key whole +// joined both keys and the separator between them into one replacement. +func TestSplitRedactionKeepsTwoSplitCredentialsOfOneShapeApart(t *testing.T) { + separators := map[string]string{"NUL": nulSeparator, "ESC": escSeparator, "zero width": zwspSeparator} + pairs := map[string][2]string{ + "jwt": {jwtToken, otherJWT}, + "github": {githubKey, "ghp_ffffffffffffffffffffffffffffffffffff"}, + "openai": {openaiKey, "sk-proj-dddddddddddddddddddddddddddddddd"}, + "anthropic": {anthropicKey, "sk-ant-api03-eeeeeeeeeeeeeeeeeeeeeeee9876543210WXYZ"}, + "slack": {slackKey, "xoxb-ANOTHER-NOT-REAL-TOKEN-BBBBBBBBBBBB"}, + } + for name, pair := range pairs { + for sepName, separator := range separators { + value := splitThroughout(pair[0], escSeparator) + separator + splitThroughout(pair[1], escSeparator) + want := RedactedSecret + separator + RedactedSecret + if got := RedactString(value, Options{}); got != want { + t.Errorf("two split %s credentials with %s between them: RedactString(...) = %q, want %q", name, sepName, got, want) + } + } + } +} + +// And a chain of them leaks nothing, however the markers fall. +func TestSplitRedactionLeavesNothingOfAChainOfSplitJWTs(t *testing.T) { + chain := splitThroughout(jwtToken, escSeparator) + nulSeparator + + splitThroughout(otherJWT, escSeparator) + nulSeparator + + splitThroughout(jwtToken, zwspSeparator) + out := rejoin(RedactString(chain, Options{})) + for _, segment := range append(strings.Split(jwtToken, "."), strings.Split(otherJWT, ".")...) { + if strings.Contains(out, segment) { + t.Errorf("a JWT segment survived a chain of three split tokens: %q in %q", segment, out) + } + } +} + +// A MATCH THE FILTER REJECTS MUST NOT HIDE THE KEY IT RAN INTO. +// +// The OpenAI shape is filtered, because kebab-case with no digit in it is prose +// and not a key. A rejected match still spans the text it matched, and its body +// runs on through a separator into whatever follows, so a split key after a +// kebab-case name was rejected along with the name and printed in full. +func TestSplitRedactionDoesNotLetARejectedNeighbourHideASplitKey(t *testing.T) { + const prose = "sk-some-kebab-case-name-here-ok" + const key = "sk-ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefgh" + if !strings.Contains(RedactString(prose, Options{}), prose) { + t.Fatal("SETUP INVALID: the prose is redacted on its own, so it is not a rejected match") + } + if strings.Contains(RedactString(key, Options{}), key) { + t.Fatal("SETUP INVALID: the key is not redacted even whole, so it is not a credential") + } + value := prose + nulSeparator + splitInTheMiddle(key, escSeparator) + want := prose + nulSeparator + RedactedSecret + if got := RedactString(value, Options{}); got != want { + t.Errorf("RedactString(...) = %q, want %q", got, want) + } +} + // ORDINARY TEXT AFTER A KEY IS NOT PART OF THE KEY. Removing the separator // between them joins the word behind it onto the end of the key, and an // unbounded shape then carries the replacement over text that was never secret. From 965e838566e0c86aa68a523d14fd0a5fe9815c07 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 24 Sep 2026 15:47:05 +0530 Subject: [PATCH 6/7] fix(redaction): recognise the next credential when its prefix is split too The run-on search looked for a shape's literal prefix contiguously after the separators, so a token split inside its first few characters was never seen as a new start and its payload leaked again. The prefix is now matched with gaps allowed between its characters, the way the shapes are. The new test splits the second token's header at every character, which also pins that the bounded search is spent only where a credential can begin. --- internal/redaction/control_split.go | 30 +++++++++++++++++++++++- internal/redaction/control_split_test.go | 15 ++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/internal/redaction/control_split.go b/internal/redaction/control_split.go index 62967f1ba..2ed71e6e5 100644 --- a/internal/redaction/control_split.go +++ b/internal/redaction/control_split.go @@ -300,7 +300,7 @@ func (shape splitShape) runOn(text string, start, end int) (next, gap, nextEnd i } at -= size } - if !strings.HasPrefix(text[candidate:], shape.prefix) { + if !hasGappedPrefix(text[candidate:], shape.prefix) { continue } tried++ @@ -311,6 +311,34 @@ func (shape splitShape) runOn(text string, start, end int) (next, gap, nextEnd i return -1, 0, 0 } +// hasGappedPrefix reports whether text opens with prefix, allowing a run of +// separators between its characters the way the shapes themselves do. A plain +// prefix test missed a token split inside its first few characters, and the run +// that swallowed it then went unnoticed. +func hasGappedPrefix(text, prefix string) bool { + at := 0 + for index, want := range prefix { + if index > 0 { + for at < len(text) { + r, size := utf8.DecodeRuneInString(text[at:]) + if !splitSecretSeparator(r) { + break + } + at += size + } + } + if at >= len(text) { + return false + } + r, size := utf8.DecodeRuneInString(text[at:]) + if r != want { + return false + } + at += size + } + return true +} + func gapTolerantAll(patterns []*regexp.Regexp) []*regexp.Regexp { tolerant := make([]*regexp.Regexp, 0, len(patterns)) for _, pattern := range patterns { diff --git a/internal/redaction/control_split_test.go b/internal/redaction/control_split_test.go index 7aacedc29..6c66a7155 100644 --- a/internal/redaction/control_split_test.go +++ b/internal/redaction/control_split_test.go @@ -342,6 +342,21 @@ func TestSplitRedactionKeepsTwoSplitCredentialsOfOneShapeApart(t *testing.T) { } } +// The search for where the next credential begins is bounded, so it has to be +// spent on positions where one can begin. Here the second token's header is split +// at every character inside the part the first match swallowed, which puts dozens +// of separator runs between the end of that match and the real boundary. +func TestSplitRedactionFindsTheNextCredentialPastItsOwnSplits(t *testing.T) { + parts := strings.Split(otherJWT, ".") + header := strings.Join(strings.Split(parts[0], ""), escSeparator) + second := header + "." + strings.Join(parts[1:], ".") + value := splitThroughout(jwtToken, escSeparator) + nulSeparator + second + want := RedactedSecret + nulSeparator + RedactedSecret + if got := RedactString(value, Options{}); got != want { + t.Errorf("RedactString(...) = %q, want %q", got, want) + } +} + // And a chain of them leaks nothing, however the markers fall. func TestSplitRedactionLeavesNothingOfAChainOfSplitJWTs(t *testing.T) { chain := splitThroughout(jwtToken, escSeparator) + nulSeparator + From fb6a1b858b31f68c51e22a9ef9cd6a2459b11c75 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 24 Sep 2026 16:05:01 +0530 Subject: [PATCH 7/7] fix(redaction): take one run-on step per match, not the whole chain Following a run-on from the second credential into a third, and on to the end of the chain, did that walk again from every match inside the chain, which is quadratic: a megabyte of split JWTs back to back allocated about seventeen gigabytes and ran for ten minutes. Each match now separates only the credential it ran into. The scan's own next match starts inside that credential and takes the following step, so a chain is still covered, and in chains of three or more the markers after the first pair can join. A test bounds the allocation on a chain of split JWTs; the linear pass uses about forty times the input and the walk used far more. --- internal/redaction/control_split.go | 27 ++++++++++++++---------- internal/redaction/control_split_test.go | 19 +++++++++++++++++ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/internal/redaction/control_split.go b/internal/redaction/control_split.go index 2ed71e6e5..dacf88bdd 100644 --- a/internal/redaction/control_split.go +++ b/internal/redaction/control_split.go @@ -251,20 +251,25 @@ const maxRunOnCandidates = 8 // they do between two whole credentials, and each piece is filtered on its own. // When the first is not complete alone, it keeps the whole of its match and the // union joins the two into one replacement. +// +// ONE STEP PER MATCH, NOT THE WHOLE CHAIN. The second credential's own match can +// run on into a third, and following that from here would walk the rest of a +// chain once for every match inside it, which is quadratic: a megabyte of split +// JWTs ran for minutes. The scan's own next match starts inside the second +// credential and takes the step after it, so the chain is still covered, and in +// a chain of three or more the markers after the first pair can join into one. func (shape splitShape) spansFor(text string, start, end int) [][]int { + next, gap, nextEnd := shape.runOn(text, start, end) + if next < 0 { + return shape.keep(nil, text, start, end) + } var spans [][]int - for { - next, gap, nextEnd := shape.runOn(text, start, end) - if next < 0 { - return shape.keep(spans, text, start, end) - } - if shape.whole.MatchString(text[start:gap]) { - spans = shape.keep(spans, text, start, gap) - } else { - spans = shape.keep(spans, text, start, end) - } - start, end = next, nextEnd + if shape.whole.MatchString(text[start:gap]) { + spans = shape.keep(spans, text, start, gap) + } else { + spans = shape.keep(spans, text, start, end) } + return shape.keep(spans, text, next, nextEnd) } func (shape splitShape) keep(spans [][]int, text string, start, end int) [][]int { diff --git a/internal/redaction/control_split_test.go b/internal/redaction/control_split_test.go index 6c66a7155..851aa0f4b 100644 --- a/internal/redaction/control_split_test.go +++ b/internal/redaction/control_split_test.go @@ -370,6 +370,25 @@ func TestSplitRedactionLeavesNothingOfAChainOfSplitJWTs(t *testing.T) { } } +// A CHAIN OF SPLIT CREDENTIALS COSTS WHAT ITS LENGTH COSTS. +// +// The run-on search once followed a chain to its end from every match inside it, +// which is quadratic in the chain: a megabyte of split JWTs back to back +// allocated seventeen gigabytes and ran for ten minutes. Measured as allocation, +// which that walk multiplies with the chain and a linear pass does not. +func TestSplitRedactionStaysLinearOnAChainOfSplitJWTs(t *testing.T) { + unit := splitThroughout(jwtToken, escSeparator) + nulSeparator + chain := strings.Repeat(unit, (64<<10)/len(unit)) + signature := strings.Split(jwtToken, ".")[2] + if strings.Contains(rejoin(RedactString(chain, Options{})), signature) { + t.Fatal("SETUP INVALID: the chain is not redacted, so the run-on search never ran") + } + limit := int64(200 * len(chain)) + if used := redactAllocBytes(chain); used > limit { + t.Errorf("redacting a %d byte chain of split JWTs allocated %d bytes, over the %d byte limit", len(chain), used, limit) + } +} + // A MATCH THE FILTER REJECTS MUST NOT HIDE THE KEY IT RAN INTO. // // The OpenAI shape is filtered, because kebab-case with no digit in it is prose