-
Notifications
You must be signed in to change notification settings - Fork 55
fix(#1230): run OutputPipeline on post-review before posting to forge #2444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,55 @@ 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) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] logging-pattern The warning log for body sanitization emits a summary count but does not log per-finding details (scanner name and detail). The established pattern in scan.go:194-196 and run.go:1831 logs each finding individually after the summary line, which helps operators diagnose what was sanitized. |
||
| if result.Sanitized != "" { | ||
| r.Body = result.Sanitized | ||
| printer.StepWarn(fmt.Sprintf("Sanitized review body (%d finding(s))", len(result.Findings))) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] repetitive-pattern The scan-check-assign pattern is repeated identically for each of the five finding fields (Severity, Category, Description, Remediation, and Body). Extracting a sanitizeField(pipeline, field) helper would reduce repetition and make it easier to add fields in the future. |
||
| } | ||
| } | ||
|
|
||
| // 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 != "" { | ||
| 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] misleading-log-message
The warning message says 'Redacted %d secret(s) in review body' but len(result.Findings) counts all pipeline findings, including those from the UnicodeNormalizer (e.g., null bytes, zero-width chars). When the body contains only zero-width obfuscation and no actual secrets, the message will still claim secrets were redacted, which is misleading for operators reviewing logs.