diff --git a/internal/githubreview/sanitize.go b/internal/githubreview/sanitize.go index da8bc8a27..28a62ac05 100644 --- a/internal/githubreview/sanitize.go +++ b/internal/githubreview/sanitize.go @@ -27,7 +27,7 @@ func SanitizeRemoteText(value string, limit int) TextEvidence { limit = MaxTextBytes } originalBytes := len([]byte(value)) - clean := outputsafe.Sanitize([]byte(value)) + clean, inputRedacted := outputsafe.SanitizeWithRedaction([]byte(value)) clean = remoteImage.ReplaceAllString(clean, "[remote image omitted]") clean = remoteLink.ReplaceAllString(clean, "$1 [remote link omitted]") clean = remoteHTMLTag.ReplaceAllString(clean, "[remote HTML omitted]") @@ -37,16 +37,17 @@ func SanitizeRemoteText(value string, limit int) TextEvidence { clean = strings.TrimSpace(clean) clean, truncated := truncateUTF8Bytes(clean, limit) redacted := redact.String(clean) - if redacted != clean { + lateRedacted := redacted != clean + if lateRedacted { clean = redacted } // A redaction marker can grow the text. Re-apply the hard byte boundary. clean, secondTruncation := truncateUTF8Bytes(clean, limit) - truncated = truncated || secondTruncation || originalBytes > len([]byte(clean)) + truncated = truncated || secondTruncation sum := Fingerprint("remote-text", clean) return TextEvidence{Text: clean, SHA256: sum, OriginalBytes: originalBytes, StoredBytes: len([]byte(clean)), Truncated: truncated, - Redacted: redacted != outputsafe.Sanitize([]byte(value)), Untrusted: true} + Redacted: inputRedacted || lateRedacted, Untrusted: true} } func EmptyTextEvidence() TextEvidence { diff --git a/internal/githubreview/sanitize_test.go b/internal/githubreview/sanitize_test.go index d93749e8a..b3673099d 100644 --- a/internal/githubreview/sanitize_test.go +++ b/internal/githubreview/sanitize_test.go @@ -23,6 +23,47 @@ func TestSanitizeRemoteTextMakesRemoteSyntaxInertAndBounded(t *testing.T) { } } +func TestSanitizeRemoteTextRedactedFlagTracksSecretRedaction(t *testing.T) { + token := "ghp_abcdefghijklmnopqrstuvwxyz0123456789" + for _, test := range []struct { + name string + value string + redacted bool + }{ + {name: "token only", value: token, redacted: true}, + {name: "inert link", value: "see [docs](https://example.com/doc)", redacted: false}, + {name: "oversized plain text", value: strings.Repeat("a", 200), redacted: false}, + } { + evidence := SanitizeRemoteText(test.value, 100) + if evidence.Redacted != test.redacted { + t.Errorf("%s: Redacted=%v, want %v (%#v)", test.name, evidence.Redacted, test.redacted, evidence) + } + } + if evidence := SanitizeRemoteText(token, 100); !strings.Contains(evidence.Text, "[REDACTED:github-token]") { + t.Fatalf("token evidence lost its redaction marker: %#v", evidence) + } +} + +// Masking a credential makes the stored evidence shorter than its input, but no +// byte bound was applied. OriginalBytes/StoredBytes already carry that difference, +// so Truncated must stay reserved for the boundary actually being reached — with +// and without the second pass that re-applies it after a growing marker. +func TestSanitizeRemoteTextTruncatedFlagTracksTheByteBound(t *testing.T) { + token := "ghp_abcdefghijklmnopqrstuvwxyz0123456789" + masked := SanitizeRemoteText("github="+token, 4096) + if masked.Truncated || !masked.Redacted || masked.StoredBytes >= masked.OriginalBytes { + t.Errorf("in-bounds redaction reported truncation: %#v", masked) + } + bounded := SanitizeRemoteText(strings.Repeat("界", 100), 40) + if !bounded.Truncated { + t.Errorf("byte bound was not reported: %#v", bounded) + } + grown := SanitizeRemoteText(strings.Repeat("a", 20)+token+strings.Repeat("b", 20), 40) + if !grown.Truncated { + t.Errorf("redaction growth past the byte bound was not reported: %#v", grown) + } +} + func TestSanitizeRemoteTextPreservesUTF8Boundary(t *testing.T) { evidence := SanitizeRemoteText(strings.Repeat("界", 10), 10) if !evidence.Truncated || !strings.HasPrefix(strings.Repeat("界", 10), evidence.Text) || diff --git a/internal/outputsafe/stream.go b/internal/outputsafe/stream.go index 0c5fad443..f794ff706 100644 --- a/internal/outputsafe/stream.go +++ b/internal/outputsafe/stream.go @@ -367,6 +367,15 @@ func (s *Stream) consume(value rune, output *strings.Builder) { // Sanitize is the one-shot form used for already bounded output. func Sanitize(data []byte) string { + text, _ := SanitizeWithRedaction(data) + return text +} + +// SanitizeWithRedaction is Sanitize plus whether secret redaction changed the +// text, so callers can tell a redaction apart from control-character removal. +func SanitizeWithRedaction(data []byte) (string, bool) { stream := &Stream{} - return redact.String(stream.Feed(data) + stream.Flush()) + inert := stream.Feed(data) + stream.Flush() + redacted := redact.String(inert) + return redacted, redacted != inert } diff --git a/internal/outputsafe/stream_test.go b/internal/outputsafe/stream_test.go index 6f7e4642b..95b8bc842 100644 --- a/internal/outputsafe/stream_test.go +++ b/internal/outputsafe/stream_test.go @@ -31,6 +31,20 @@ func TestSanitizeRemovesC1UnicodeControlsAndRedactsSecrets(t *testing.T) { } } +func TestSanitizeWithRedactionSeparatesRedactionFromControlStripping(t *testing.T) { + secret := []byte("before\u009b31mafter token=secret-value-1234567890") + got, redacted := SanitizeWithRedaction(secret) + if !redacted || got != Sanitize(secret) { + t.Fatalf("redaction was not reported apart from stripping: %q %v", got, redacted) + } + // Removing control characters shortens the text but is not a secret redaction. + plain := []byte("before\u009b31mafter\u009dtitle") + got, redacted = SanitizeWithRedaction(plain) + if redacted || got != Sanitize(plain) { + t.Fatalf("control stripping must not report redaction: %q %v", got, redacted) + } +} + func TestRedactingStreamCarriesSecretAndPrivateKeyStateAcrossChunks(t *testing.T) { stream := &RedactingStream{} parts := []string{