From db753f51b641ebbc27da5ab16560bdff5695ad42 Mon Sep 17 00:00:00 2001 From: Qoder-AI <215799558+Qoder-AI@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:12:28 +0800 Subject: [PATCH 1/2] fix(githubreview): report redaction separately from inert escaping SanitizeRemoteText decided its Redacted flag by comparing the redacted text with outputsafe.Sanitize of the raw input, but Sanitize already runs redact.String and the escaping pipeline had rewritten the other side. A redacted GitHub token therefore reported Redacted=false while a benign link or an oversized plain body reported true. Let outputsafe say whether redaction changed the text and keep Sanitize's output identical by construction. --- internal/githubreview/sanitize.go | 7 ++++--- internal/githubreview/sanitize_test.go | 21 +++++++++++++++++++++ internal/outputsafe/stream.go | 11 ++++++++++- internal/outputsafe/stream_test.go | 14 ++++++++++++++ 4 files changed, 49 insertions(+), 4 deletions(-) diff --git a/internal/githubreview/sanitize.go b/internal/githubreview/sanitize.go index da8bc8a27..6842af6c2 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,7 +37,8 @@ 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. @@ -46,7 +47,7 @@ func SanitizeRemoteText(value string, limit int) TextEvidence { 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..b95fdcabf 100644 --- a/internal/githubreview/sanitize_test.go +++ b/internal/githubreview/sanitize_test.go @@ -23,6 +23,27 @@ 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) + } +} + 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{ From 90fba72e40e892150fece44501cc0e23f9b0dda3 Mon Sep 17 00:00:00 2001 From: Qoder-AI <215799558+Qoder-AI@users.noreply.github.com> Date: Sat, 24 Oct 2026 03:12:39 +0800 Subject: [PATCH 2/2] fix(githubreview): report truncation only when the byte bound was applied SanitizeRemoteText also ORed "the stored text is shorter than the input" into Truncated, so a 47-byte payload holding one credential reported Truncated=true against a 4096-byte bound simply because masking made it shorter. The two real truncation passes already track the boundary, and OriginalBytes plus StoredBytes carry any shrinkage, so reserve Truncated for the limit actually being reached. --- internal/githubreview/sanitize.go | 2 +- internal/githubreview/sanitize_test.go | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/internal/githubreview/sanitize.go b/internal/githubreview/sanitize.go index 6842af6c2..28a62ac05 100644 --- a/internal/githubreview/sanitize.go +++ b/internal/githubreview/sanitize.go @@ -43,7 +43,7 @@ func SanitizeRemoteText(value string, limit int) TextEvidence { } // 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, diff --git a/internal/githubreview/sanitize_test.go b/internal/githubreview/sanitize_test.go index b95fdcabf..b3673099d 100644 --- a/internal/githubreview/sanitize_test.go +++ b/internal/githubreview/sanitize_test.go @@ -44,6 +44,26 @@ func TestSanitizeRemoteTextRedactedFlagTracksSecretRedaction(t *testing.T) { } } +// 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) ||