Repository navigation
fix: sanitize secret-bearing data before writing GitHub step summaries - #50777
Conversation
- parse_copilot_log.cjs: disable includeDetailedParameters to stop rendering raw tool inputs/outputs in step summaries - safe_output_summary.cjs: remove body preview (message.body/result.body), message.data rendering, and raw JSON.stringify(message) from error path to prevent secret leakage into step summaries - Update tests to reflect new security-safe behavior and add sentinel secret exclusion tests covering body, data, error, multiline, URL, and base64-encoded shapes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Raw tool outputs, input-derived summaries, and exception messages can still expose secrets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR reduces secret exposure in GitHub step summaries.
Changes:
- Hides detailed tool parameters.
- Removes raw safe-output bodies, data, and message details.
- Adds secret-exclusion tests.
File summaries
| File | Description |
|---|---|
parse_copilot_log.cjs |
Disables detailed tool parameters. |
parse_copilot_log.test.cjs |
Updates parser expectations. |
safe_output_summary.cjs |
Removes unsafe payload rendering. |
safe_output_summary.test.cjs |
Adds secret-exclusion coverage. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| } | ||
| } else if (error) { | ||
| // Show error information | ||
| // Show error information (raw message content is omitted to prevent secret leakage) |
There was a problem hiding this comment.
Fixed. The error path now renders only an allowlisted error code — the leading ERR_* / E0xx prefix when it matches a code exported from error_codes.cjs, otherwise UNCLASSIFIED — followed by a pointer to the job logs. Raw exception text is never written to the summary.
Added sentinel tests that place the secret directly in error, for both a classified (ERR_API: ... token <SENTINEL>) and an unclassified (Handler failed: <SENTINEL>) message, and updated the existing tests that asserted on raw error text.
| // Generate conversation markdown using shared function | ||
| const conversationResult = generateConversationMarkdown(canonicalLogEntries, { | ||
| formatToolCallback: (toolUse, toolResult) => formatToolUse(toolUse, toolResult, { includeDetailedParameters: true }), | ||
| formatToolCallback: (toolUse, toolResult) => formatToolUse(toolUse, toolResult, { includeDetailedParameters: false }), |
There was a problem hiding this comment.
You're right that disabling includeDetailedParameters alone doesn't close this — and the gap is actually wider than formatToolUse: the primary step-summary path in log_parser_bootstrap.cjs is generateCopilotCliStyleSummary, not the formatToolUse markdown (which is only used on the fallback path). That renderer also emits commands, result previews, and agent text, so a summary-safe mode in formatToolUse would have left the main path leaking.
I didn't strip tool inputs/outputs from the rendering, because reproducing them is the point of the agent log summary — removing them would make run triage impossible while still not covering agent-authored text, which can quote a secret just as easily. Instead the redaction is applied at the sink: a shared redactStepSummaryContent helper in redact_secrets.cjs runs the same built-in credential patterns already used for artifact redaction over the markdown immediately before core.summary.addRaw, on the structured path, the fallback path, and the Claude startup-diagnostics path, plus the safe-output summary writer (whose title/label/URL fields are agent-controlled).
Sentinel tests added in log_parser_bootstrap.test.cjs cover a credential in a tool input (Bash command) and in a tool output (tool_result content), on both the structured and fallback paths, asserting the value is replaced by ***REDACTED***.
One limitation worth flagging: this covers credential-shaped values, not workflow-configured secret values, because GH_AW_SECRET_NAMES / SECRET_* are only wired into the dedicated redaction step. Passing those into the log-parser and safe-outputs steps is a compiler change with its own exposure trade-off, so I left it out of this PR.
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🚀 This PR looks great! You're addressing a real security vulnerability — preventing secrets from leaking into GitHub Actions step summaries. The approach is sound:
This is ready for review and merge. Well done! 👍
|
|
@copilot run pr-finisher skill |
…mmaries Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Several paths serialized agent-controlled data directly into
$GITHUB_STEP_SUMMARY— tool inputs/outputs viaincludeDetailedParameters: true, and rawmessage.body,message.data, and fullJSON.stringify(message)in safe-output summaries. Actions::add-mask::processing does not reliably scrub step-summary file contents, so secrets could persist in cleartext.Changes
parse_copilot_log.cjsincludeDetailedParameters— tool call rendering now shows only tool name, command summary, duration, and status; raw parameter JSON is never written to the summarysafe_output_summary.cjsmessage.body/result.body) from success and fallback pathsmessage.datablock (arbitrary structured agent output)JSON.stringify(message, null, 2)from the error pathtitle,labels,url,repo#number,secrecy,integrityTests
sentinel secret exclusionsuite assertingSUPERSECRETVALUE_xK9mQ2wRis absent from generated markdown across:message.body,result.body,message.data, error path, multiline strings, URL-embedded values, and base64-encoded values