From f222a7dfde63183c46b0071f743c342d1b912403 Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 18 Jun 2026 19:56:17 +0000 Subject: [PATCH 1/3] fix(#1230): run OutputPipeline on post-review before posting to forge The post-review command posted review content directly to the GitHub API without running it through the security output pipeline. When invoked standalone (outside fullsend run), secrets and zero-width- obfuscated tokens in agent output could reach the forge unredacted. Call security.OutputPipeline().Scan() on the review body and finding text fields (description, remediation) before any forge API call. This matches the pattern used by fullsend scan output and the sandbox post-tool hooks. Co-Authored-By: Claude Opus 4.6 --- internal/cli/postreview.go | 44 +++++++++++++++++ internal/cli/postreview_test.go | 87 +++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index 6ef89a7ae..2ecb3b018 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -13,6 +13,7 @@ import ( "github.com/fullsend-ai/fullsend/internal/forge" gh "github.com/fullsend-ai/fullsend/internal/forge/github" + "github.com/fullsend-ai/fullsend/internal/security" "github.com/fullsend-ai/fullsend/internal/sticky" "github.com/fullsend-ai/fullsend/internal/ui" ) @@ -87,6 +88,12 @@ has moved, a stale-head failure is posted instead.`, return fmt.Errorf("parsing review result: %w", err) } + // Sanitize review content through the output security + // pipeline before posting to the forge. This redacts + // leaked secrets and normalizes zero-width unicode + // obfuscation that could bypass pattern-based redaction. + parsed = sanitizeReviewResult(parsed, printer) + // CLI flag takes precedence over JSON field. if headSHA != "" { parsed.HeadSHA = headSHA @@ -527,6 +534,43 @@ func minimizeStaleReviews(ctx context.Context, client forge.Client, user string, printer.StepDone("Stale reviews minimized") } +// sanitizeReviewResult runs the security output pipeline over all +// user-visible text fields in a ReviewResult. This catches leaked +// secrets and zero-width–obfuscated tokens before they reach the +// forge API. +func sanitizeReviewResult(r ReviewResult, printer *ui.Printer) ReviewResult { + pipeline := security.OutputPipeline() + + // Sanitize the main body. + if r.Body != "" { + result := pipeline.Scan(r.Body) + if result.Sanitized != "" { + r.Body = result.Sanitized + printer.StepWarn(fmt.Sprintf("Redacted %d secret(s) in review body", len(result.Findings))) + } + } + + // Sanitize finding descriptions and remediations — these are + // posted as inline PR review comments and could carry secrets + // from agent output. + for i := range r.Findings { + if r.Findings[i].Description != "" { + result := pipeline.Scan(r.Findings[i].Description) + if result.Sanitized != "" { + r.Findings[i].Description = result.Sanitized + } + } + if r.Findings[i].Remediation != "" { + result := pipeline.Scan(r.Findings[i].Remediation) + if result.Sanitized != "" { + r.Findings[i].Remediation = result.Sanitized + } + } + } + + return r +} + // parseReviewResult attempts to parse the body as a JSON ReviewResult. // If parsing fails, treats the entire input as a plain-text body. // Returns an error if the JSON is valid but the body field is empty diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 5be6ac4be..2d5fe2c39 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -1044,6 +1044,93 @@ func TestFormatFindingComment(t *testing.T) { }) } +func TestSanitizeReviewResult_RedactsSecretsInBody(t *testing.T) { + printer := ui.New(io.Discard) + secret := "ghp_FAKEtesttoken000000000000000000000000" + r := ReviewResult{ + Body: "Found this token: " + secret + " in the code.", + Action: "comment", + } + + sanitized := sanitizeReviewResult(r, printer) + assert.NotContains(t, sanitized.Body, "ghp_FAKEtest", "secret should be redacted from body") + assert.Contains(t, sanitized.Body, "Found this token:", "non-secret text should remain") +} + +func TestSanitizeReviewResult_RedactsSecretsInFindings(t *testing.T) { + printer := ui.New(io.Discard) + secret := "ghp_FAKEtesttoken000000000000000000000000" + r := ReviewResult{ + Body: "Review body without secrets.", + Action: "request-changes", + Findings: []ReviewFinding{ + { + Severity: "high", + Category: "security", + File: "main.go", + Line: 10, + Description: "Hardcoded token: " + secret, + Remediation: "Remove " + secret + " and use env var.", + }, + }, + } + + sanitized := sanitizeReviewResult(r, printer) + assert.NotContains(t, sanitized.Findings[0].Description, "ghp_FAKEtest", "secret should be redacted from finding description") + assert.NotContains(t, sanitized.Findings[0].Remediation, "ghp_FAKEtest", "secret should be redacted from finding remediation") + assert.Contains(t, sanitized.Findings[0].Description, "Hardcoded token:", "non-secret text should remain") +} + +func TestSanitizeReviewResult_ZeroWidthObfuscatedSecret(t *testing.T) { + printer := ui.New(io.Discard) + plain := "ghp_FAKEtesttoken000000000000000000000000" + // Interleave zero-width non-joiner characters to obfuscate the token. + var obfuscated string + for _, c := range plain { + obfuscated += string(c) + "\u200c" + } + r := ReviewResult{ + Body: "Token: " + obfuscated, + Action: "comment", + } + + sanitized := sanitizeReviewResult(r, printer) + assert.NotContains(t, sanitized.Body, "ghp_FAKEtest", "zero-width obfuscated secret should be caught after normalization") +} + +func TestSanitizeReviewResult_NoSecretsPassesThrough(t *testing.T) { + printer := ui.New(io.Discard) + r := ReviewResult{ + Body: "Looks good! No issues found.", + Action: "approve", + Findings: []ReviewFinding{ + { + Severity: "low", + Category: "style", + File: "main.go", + Line: 5, + Description: "Consider renaming variable.", + }, + }, + } + + sanitized := sanitizeReviewResult(r, printer) + assert.Equal(t, "Looks good! No issues found.", sanitized.Body, "clean body should pass through unchanged") + assert.Equal(t, "Consider renaming variable.", sanitized.Findings[0].Description, "clean finding should pass through unchanged") +} + +func TestSanitizeReviewResult_EmptyBody(t *testing.T) { + printer := ui.New(io.Discard) + r := ReviewResult{ + Body: "", + Action: "failure", + Reason: "tool-failure", + } + + sanitized := sanitizeReviewResult(r, printer) + assert.Empty(t, sanitized.Body, "empty body should remain empty") +} + func TestPostApprovedFollowUpIssues_DisabledIsNoop(t *testing.T) { // Issue creation is disabled (#1137). Verify the function is a no-op for // approve actions with actionable findings. From 037d0233c64ded656a852cd354b96c6532e6f1dd Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Mon, 22 Jun 2026 20:15:36 +0000 Subject: [PATCH 2/3] fix(#1230): sanitize severity/category fields and fix misleading log message Address two review findings on PR #2444: 1. [incomplete-sanitization] Apply pipeline.Scan() to Severity and Category fields in ReviewFinding, which are interpolated into Markdown posted to the forge by formatFindingComment. 2. [misleading-log-message] Change warning from "Redacted N secret(s)" to "Sanitized review body (N finding(s))" since pipeline findings include unicode normalization, not just secrets. Addresses review feedback on #2444 --- internal/cli/postreview.go | 20 ++++++++++++++++---- internal/cli/postreview_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index 2ecb3b018..e48c720cd 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -546,14 +546,26 @@ func sanitizeReviewResult(r ReviewResult, printer *ui.Printer) ReviewResult { result := pipeline.Scan(r.Body) if result.Sanitized != "" { r.Body = result.Sanitized - printer.StepWarn(fmt.Sprintf("Redacted %d secret(s) in review body", len(result.Findings))) + printer.StepWarn(fmt.Sprintf("Sanitized review body (%d finding(s))", len(result.Findings))) } } - // Sanitize finding descriptions and remediations — these are - // posted as inline PR review comments and could carry secrets - // from agent output. + // Sanitize finding fields — severity, category, description, and + // remediation are all interpolated into Markdown posted to the + // forge and could carry secrets from agent output. for i := range r.Findings { + if r.Findings[i].Severity != "" { + result := pipeline.Scan(r.Findings[i].Severity) + if result.Sanitized != "" { + r.Findings[i].Severity = result.Sanitized + } + } + if r.Findings[i].Category != "" { + result := pipeline.Scan(r.Findings[i].Category) + if result.Sanitized != "" { + r.Findings[i].Category = result.Sanitized + } + } if r.Findings[i].Description != "" { result := pipeline.Scan(r.Findings[i].Description) if result.Sanitized != "" { diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 2d5fe2c39..3e571dec3 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -1081,6 +1081,28 @@ func TestSanitizeReviewResult_RedactsSecretsInFindings(t *testing.T) { assert.Contains(t, sanitized.Findings[0].Description, "Hardcoded token:", "non-secret text should remain") } +func TestSanitizeReviewResult_RedactsSecretsInSeverityAndCategory(t *testing.T) { + printer := ui.New(io.Discard) + secret := "ghp_FAKEtesttoken000000000000000000000000" + r := ReviewResult{ + Body: "Review body without secrets.", + Action: "request-changes", + Findings: []ReviewFinding{ + { + Severity: "high " + secret, + Category: "security " + secret, + File: "main.go", + Line: 10, + Description: "Clean description.", + }, + }, + } + + sanitized := sanitizeReviewResult(r, printer) + assert.NotContains(t, sanitized.Findings[0].Severity, "ghp_FAKEtest", "secret should be redacted from finding severity") + assert.NotContains(t, sanitized.Findings[0].Category, "ghp_FAKEtest", "secret should be redacted from finding category") +} + func TestSanitizeReviewResult_ZeroWidthObfuscatedSecret(t *testing.T) { printer := ui.New(io.Discard) plain := "ghp_FAKEtesttoken000000000000000000000000" @@ -1117,6 +1139,8 @@ func TestSanitizeReviewResult_NoSecretsPassesThrough(t *testing.T) { sanitized := sanitizeReviewResult(r, printer) assert.Equal(t, "Looks good! No issues found.", sanitized.Body, "clean body should pass through unchanged") assert.Equal(t, "Consider renaming variable.", sanitized.Findings[0].Description, "clean finding should pass through unchanged") + assert.Equal(t, "low", sanitized.Findings[0].Severity, "clean severity should pass through unchanged") + assert.Equal(t, "style", sanitized.Findings[0].Category, "clean category should pass through unchanged") } func TestSanitizeReviewResult_EmptyBody(t *testing.T) { From 4cb8250e358b3b0f09cc66374a8e0d3ceaecd710 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Mon, 22 Jun 2026 21:03:12 +0000 Subject: [PATCH 3/3] fix: log per-finding details after body sanitization summary Add individual finding logging (scanner name + detail) after the summary count line in sanitizeReviewResult, matching the established pattern in scan.go:194-196 and run.go:1831. Addresses review feedback on #2444 --- internal/cli/postreview.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index e48c720cd..819760710 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -547,6 +547,9 @@ func sanitizeReviewResult(r ReviewResult, printer *ui.Printer) ReviewResult { if result.Sanitized != "" { r.Body = result.Sanitized printer.StepWarn(fmt.Sprintf("Sanitized review body (%d finding(s))", len(result.Findings))) + for _, f := range result.Findings { + printer.StepWarn(fmt.Sprintf(" %s: %s", f.Name, f.Detail)) + } } }