fix(redaction): replace only the JWT when it is followed by a JSON escape (#3508)

Author: migmartri

internal/redaction/betterleaks.go

@@ -332,12 +332,46 @@ func byteSpans(text string, ranges []lineSpan) []span {
 // appendSecret records a locatable secret. A finding without one cannot be
 // searched for in the document, so it is dropped rather than reported.
 func appendSecret(dst []Finding, ruleID, secret string) []Finding {
+	if ruleID == jwtRuleID {
+		secret = cutAtTerminatorEscape(secret)
+	}
 	if secret == "" {
 		return dst
 	}
 	return append(dst, Finding{RuleID: ruleID, Secret: secret})
 }
 
+// jwtRuleID is the default ruleset's rule for JSON Web Tokens.
+const jwtRuleID = "jwt"
+
+// cutAtTerminatorEscape removes the text from the first whitespace or quote
+// escape onwards.
+//
+// The jwt rule allows a backslash in its last two segments, so in JSON-encoded
+// text its match runs through an escape such as `\n` and into the word after
+// it. Replacing that match would remove text that is not part of the token. A
+// JWT is base64url, so the part before such an escape is the whole token. Other
+// escapes, such as `\/` or `\\`, are kept: they are not proof that the token has
+// ended, and keeping them can only over-redact.
+//
+// Escapes are read as units from the left, so the second backslash of a `\\`
+// escape is never taken as the start of another escape.
+func cutAtTerminatorEscape(secret string) string {
+	for i := 0; i+1 < len(secret); i++ {
+		if secret[i] != '\\' {
+			continue
+		}
+		switch secret[i+1] {
+		case '"', 'n', 'r', 't':
+			return secret[:i]
+		}
+		// Skip the escaped character, so that it is not read as the start of
+		// an escape.
+		i++
+	}
+	return secret
+}
+
 // span is a half-open byte range [start, end) of a text.
 type span struct {
 	start, end int

internal/redaction/betterleaks_test.go

@@ -17,6 +17,7 @@ package redaction
 
 import (
 	"context"
+	"encoding/json"
 	"regexp"
 	"strconv"
 	"strings"
@@ -42,6 +43,7 @@ const (
 	fakeAWSKey    = "AKIA" + "4G7TI63VCBIRS4GW"
 	fakeAWSSecret = "kQ7zXn2VbW9pLm4RtY6" + "uHs3JdF8gA1cE5oPzQwXn"
 	fakeGitHubPAT = "ghp_erOZlZv0B1e3amrQ" + "ugdwZ8Ro2W4kDql9WPTf"
+	fakeJWT       = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9" + ".eyJzdWIiOiJmYWtlLXVwbG9hZCIsImV4cCI6MTc5MDAwMDAwMH0" + ".c2lnbmF0dXJlLWZha2UtZm9yLXJlcHJv"
 )
 
 var fakeAnthropicKey = "sk-ant-api03-" + strings.Repeat("a", 93) + "AA"
@@ -279,6 +281,101 @@ func TestRedactCredentialInURIConverges(t *testing.T) {
 	assert.False(t, report.Changed())
 }
 
+// TestRedactJWTKeepsSurroundingText covers a JWT that is followed by a JSON
+// escape sequence in its string leaf. The jwt rule allows a backslash in its
+// last segments, so the scanner reports the secret with the backslash that
+// starts the escape. Only the JWT must be replaced: the text around it, such as
+// the host of a presigned URL, is what lets policies tell a short-lived URL
+// signature from a leaked credential.
+func TestRedactJWTKeepsSurroundingText(t *testing.T) {
+	scanner, err := DefaultScanner()
+	require.NoError(t, err)
+
+	testCases := []struct {
+		name string
+		leaf string
+		want string
+	}{
+		{
+			// A tool result that holds a JSON document as text: the quote after
+			// the JWT is escaped in the leaf.
+			name: "followed by an escaped quote in a nested JSON string",
+			leaf: `{"url":"https://uploads.example.com/o/a/b?signature=` + fakeJWT + `"}`,
+			want: `{"url":"https://uploads.example.com/o/a/b?signature=[REDACTED:jwt]"}`,
+		},
+		{
+			name: "followed by an escaped newline",
+			leaf: "https://uploads.example.com/o/a/b?signature=" + fakeJWT + "\nnext line",
+			want: "https://uploads.example.com/o/a/b?signature=[REDACTED:jwt]\nnext line",
+		},
+		{
+			name: "followed by an escaped carriage return",
+			leaf: "https://uploads.example.com/o/a/b?signature=" + fakeJWT + "\rnext line",
+			want: "https://uploads.example.com/o/a/b?signature=[REDACTED:jwt]\rnext line",
+		},
+		{
+			name: "followed by an escaped tab",
+			leaf: "https://uploads.example.com/o/a/b?signature=" + fakeJWT + "\tnext line",
+			want: "https://uploads.example.com/o/a/b?signature=[REDACTED:jwt]\tnext line",
+		},
+		{
+			// The literal backslash is a complete `\\` escape, which is kept in
+			// the secret: the leaf loses that one character but not its context.
+			name: "followed by a literal backslash",
+			leaf: `https://uploads.example.com/o/a/b?signature=` + fakeJWT + `\"`,
+			want: `https://uploads.example.com/o/a/b?signature=[REDACTED:jwt]"`,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			doc, err := json.Marshal(map[string]string{"text": tc.leaf})
+			require.NoError(t, err)
+
+			once, report, err := New(scanner).Redact(context.Background(), doc)
+			require.NoError(t, err)
+			require.True(t, report.Changed())
+			assert.Equal(t, map[string]int{"jwt": 1}, report.ByRule)
+
+			var got map[string]string
+			require.NoError(t, json.Unmarshal(once, &got))
+			assert.Equal(t, tc.want, got["text"], "only the JWT is replaced")
+
+			twice, report, err := New(scanner).Redact(context.Background(), once)
+			require.NoError(t, err)
+			assert.Equal(t, string(once), string(twice))
+			assert.False(t, report.Changed())
+		})
+	}
+}
+
+// TestCutAtTerminatorEscape checks that the cut is made only at a real escape.
+// In JSON-encoded text a backslash that belongs to a `\\` escape does not start
+// another escape, so `\\n` is a literal backslash followed by the letter n.
+func TestCutAtTerminatorEscape(t *testing.T) {
+	testCases := []struct {
+		name   string
+		secret string
+		want   string
+	}{
+		{name: "no escape", secret: `plain`, want: `plain`},
+		{name: "escaped quote", secret: `quote\"after`, want: `quote`},
+		{name: "escaped newline", secret: `newline\nafter`, want: `newline`},
+		{name: "escaped carriage return", secret: `return\rafter`, want: `return`},
+		{name: "escaped tab", secret: `tab\tafter`, want: `tab`},
+		{name: "escaped backslash then n is not a newline", secret: `bs\\nafter`, want: `bs\\nafter`},
+		{name: "escaped backslash then quote escape", secret: `bsquote\\\"after`, want: `bsquote\\`},
+		{name: "escaped slash is kept", secret: `slash\/kept\nafter`, want: `slash\/kept`},
+		{name: "trailing lone backslash is left to the engine", secret: `lone\`, want: `lone\`},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.want, cutAtTerminatorEscape(tc.secret))
+		})
+	}
+}
+
 // TestDefaultScannerUsesRE2 pins the regex engine. The library default is the
 // standard library engine, which is several times slower on the default ruleset
 // and made large sessions take tens of minutes to redact.

internal/redaction/redaction.go

@@ -312,23 +312,24 @@ func pendingSecrets(findings []Finding, skip map[string]struct{}, isPlaceholder
 	out := make([]secretRule, 0, len(findings))
 
 	for _, f := range findings {
-		if f.Secret == "" {
+		secret := trimPartialEscape(f.Secret)
+		if secret == "" {
 			continue
 		}
-		if _, skipped := skip[f.Secret]; skipped {
+		if _, skipped := skip[secret]; skipped {
 			continue
 		}
 		// A finding whose whole secret is a placeholder is a rule matching the
 		// position it sits in, not a credential. Rewriting it would churn the
 		// document on every run and never terminate.
-		if isPlaceholder != nil && isPlaceholder(f.Secret) {
+		if isPlaceholder != nil && isPlaceholder(secret) {
 			continue
 		}
-		if _, dup := seen[f.Secret]; dup {
+		if _, dup := seen[secret]; dup {
 			continue
 		}
-		seen[f.Secret] = struct{}{}
-		out = append(out, secretRule{secret: f.Secret, ruleID: f.RuleID})
+		seen[secret] = struct{}{}
+		out = append(out, secretRule{secret: secret, ruleID: f.RuleID})
 	}
 
 	sort.Slice(out, func(i, j int) bool {
@@ -340,6 +341,24 @@ func pendingSecrets(findings []Finding, skip map[string]struct{}, isPlaceholder
 	return out
 }
 
+// trimPartialEscape removes a trailing backslash that starts a JSON escape
+// sequence the secret does not include.
+//
+// The scanner sees JSON-encoded text, where every backslash starts or completes
+// an escape. A rule whose character class allows a backslash can end its match
+// on the first character of an escape such as `\"` or `\n`. Replacing that
+// secret would leave the rest of the escape behind, the leaf would no longer
+// decode, and the whole leaf would be replaced. An odd run of trailing
+// backslashes is the sign of that: an even run is a set of complete `\\`
+// escapes, which are part of the secret.
+func trimPartialEscape(secret string) string {
+	run := len(secret) - len(strings.TrimRight(secret, `\`))
+	if run%2 == 1 {
+		return secret[:len(secret)-1]
+	}
+	return secret
+}
+
 // rewriter walks a decoded JSON value tree replacing secrets in eligible string
 // leaves.
 type rewriter struct {

internal/redaction/redaction_test.go

@@ -150,6 +150,22 @@ func TestRedact(t *testing.T) {
 			mustNotContain:   []string{"before", "after"},
 			mustContain:      []string{"[REDACTED:r1]"},
 		},
+		{
+			name:             "secret ending on the backslash of an escape keeps the leaf",
+			doc:              `{"a":"{\"url\":\"https://h/x?sig=SEC\"}"}`,
+			findings:         []Finding{{RuleID: "r1", Secret: `SEC\`}},
+			wantReplacements: 1,
+			wantByRule:       map[string]int{"r1": 1},
+			mustContain:      []string{`{"a":"{\"url\":\"https://h/x?sig=[REDACTED:r1]\"}"}`},
+		},
+		{
+			name:             "secret ending on a complete escaped backslash keeps it",
+			doc:              `{"a":"SEC\\ after"}`,
+			findings:         []Finding{{RuleID: "r1", Secret: `SEC\\`}},
+			wantReplacements: 1,
+			wantByRule:       map[string]int{"r1": 1},
+			mustContain:      []string{`{"a":"[REDACTED:r1] after"}`},
+		},
 		{
 			name:     "protected path is left alone and recorded",
 			doc:      `{"keepme":"SEC","other":"plain"}`,