fix(pre-code): harden GHA workflow command injection surface - #576
fix(pre-code): harden GHA workflow command injection surface#576ralphbean wants to merge 8 commits into
Conversation
Move the ::notice:: for GITHUB_ISSUE_URL after input validation so unvalidated values never reach GHA workflow command parsing. Replace raw COMMENT_BODY echo with body length to avoid user-controlled content hitting stdout where GHA scans for :: commands. Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
The source file pre-code.src.sh was modified but the bundled output was not regenerated, causing the check-bundle CI step to fail. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by QodoHarden pre-code GitHub Actions logging against workflow command injection
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
🤖 Finished Review · ✅ Success · Started 8:49 PM UTC · Completed 9:02 PM UTC |
Code Review by Qodo
1. Protected scripts/ files modified
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsHigh
Low
Next steps:
Previous runReviewFindingsHigh
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Low
Labels: PR hardens GHA workflow command injection surface in pre-code scripts — security-relevant change Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 9:03 PM UTC · Completed 9:06 PM UTC |
🔧 Fix agent — iteration 1 (bot-triggered)Both review findings are governance/procedural or out-of-scope suggestions rather than code defects. Finding 1 (protected-path) requires human approval, which the fix agent cannot provide. Finding 2 (output-consistency) suggests extending changes to other pre-scripts, which is out of scope for this PR. No code changes were made. Disagreed (2):
Tests: passedNext steps:
|
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review finding
[HIGH] scripts/pre-code.src.sh (lines 28, 33, 38, 46, 50) — Validation-failure ::error:: lines still echo raw untrusted input, the same injection class this PR claims to fix
This PR moves the GITHUB_ISSUE_URL ::notice:: to after validation and stops echoing raw COMMENT_BODY, specifically so unvalidated/attacker-controlled values never reach GHA workflow-command parsing. But five untouched lines in the same validation block still interpolate the raw, already-known-invalid value directly into ::error:: lines:
- line 28:
echo "::error::ISSUE_NUMBER must be a positive integer, got: '${ISSUE_NUMBER:-}'" - line 33: REPO_FULL_NAME format error
- line 38: GITHUB_ISSUE_URL format error
- line 46 and line 50: the URL-vs-input cross-check mismatches
Each of these fires exactly when the value has failed its regex check or cross-check — i.e., precisely the malformed/malicious event_payload class the script's own header comment says it exists to guard against. GHA's workflow-command parser scans every stdout line for a leading ::; a value containing an embedded literal newline followed by ::something:: (e.g. ISSUE_NUMBER, REPO_FULL_NAME, or GITHUB_ISSUE_URL supplied via workflow_dispatch with such content) would inject a second workflow command on the following log line, regardless of being nested inside another ::error:: message.
These lines are unchanged by the PR's diff (and not visible in the diff's context window, hence this is a whole-review comment rather than an inline one) and are not covered by any existing review comment — those only address the protected-path warning, missing test coverage, and the other pre-scripts' notice ordering.
Suggestion: Apply the same treatment used for COMMENT_BODY here — don't interpolate raw untrusted values directly into ::error:: lines. Strip/replace embedded newlines and ::-like sequences before interpolating (e.g. ${VALUE//$'\n'/ }), or follow GitHub's documented escaping for workflow commands (%→%25, \r→%0D, \n→%0A), applied consistently across all five sites (scripts/pre-code.src.sh:28,33,38,46,50, mirrored in scripts/pre-code.sh).
Five ::error:: lines interpolated raw, already-invalid ISSUE_NUMBER, REPO_FULL_NAME, and GITHUB_ISSUE_URL values — the same workflow-command injection class this PR already fixed for the ::notice:: line and COMMENT_BODY logging. Route them through the existing gha_echo sanitizer (scripts/lib/post-failure-report.lib.sh) instead of hand-rolling a new one, matching the convention pr-assignee.lib.sh already documents. Also adds regression tests for the ::notice:: reordering and the COMMENT_BODY length logging that this PR introduced but didn't test. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
@waynesun09 good eye — same injection class, just five lines that weren't in this diff's context window. Pushed 90f03f3, which routes those through Also turned up the same pattern in |
|
🤖 Finished Review · ✅ Success · Started 1:29 PM UTC · Completed 1:46 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 1:48 PM UTC · Completed 1:54 PM UTC |
Route CODE_FORCE through sanitize_gha_log_output before interpolation in the force-override debug echo, closing the remaining unsanitized workflow-command injection vector flagged in review. Addresses review feedback on #576
🔧 Fix agent — iteration 1 (bot-triggered)Addressed 1 of 3 review findings with a code fix. Disagreed with 2: [protected-path] requires human approval (procedural, not a code fix), and [edge-case] is an acknowledged security-over-debuggability tradeoff with no remediation requested. The [injection] finding was valid — CODE_FORCE was the last unsanitized interpolation in the force-override debug log; now routed through sanitize_gha_log_output consistent with the PR's existing COMMENT_BODY hardening pattern. Fixed (1):
Disagreed (2):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 1:56 PM UTC · Completed 2:08 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 2:09 PM UTC · Completed 2:15 PM UTC |
🔧 Fix agent — iteration 2 (bot-triggered)Disagreed with both remaining review findings. [protected-path] is a procedural governance requirement that requires human approval — not a code fix. [edge-case] is an acknowledged security-over-debuggability tradeoff with no remediation requested by the reviewer. The [injection] finding from the previous review cycle was already fixed in iteration 1 and is no longer raised. Disagreed (2):
Tests: passedNext steps:
|
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review finding
[HIGH] scripts/pre-review.sh (lines 17, 22, 27, 35, 39) and scripts/pre-fix.sh (lines 33, 38) — Identical unpatched workflow-command injection remains in sibling pre-scripts
This PR's stated purpose is to harden pre-script GHA workflow-command injection surface, and it correctly converts pre-code.src.sh's ::error:: validation lines to gha_echo. But pre-review.sh (PR_NUMBER, REPO_FULL_NAME, GITHUB_PR_URL, and the two cross-check mismatch lines) and pre-fix.sh (PR_NUMBER, REPO_FULL_NAME) still use raw echo "::error::... got: '${VALUE:-}'", interpolating the same not-yet-trusted, just-failed-validation input directly into a ::-prefixed line. Verified against file contents at head commit 16d120d — both files are untouched by this PR (hence a whole-review comment rather than inline, since neither file appears in this PR's diff) and neither is bundled from a .src.sh source. This is the exact vulnerability class the PR exists to fix, left open in two of the other pre-scripts, and isn't covered by the existing maintainer response (which only scoped out the unrelated ::notice:: reordering cosmetic issue, not this injection vector).
Suggestion: Either narrow the PR title/description to scope the fix explicitly to pre-code (noting the identical exposure remains elsewhere), or apply the same gha_echo-based sanitization to the equivalent lines in pre-review.sh and pre-fix.sh in this PR or an immediate follow-up tracked by an issue.
Review finding
[MEDIUM] PR description Summary omits half of the actual diff's hardening changes
Verified via gh pr view 576: the Summary lists only two changes (move ::notice:: after validation; replace raw COMMENT_BODY echo with length). The actual diff at head also converts all five ::error:: validation lines to gha_echo and sanitizes CODE_FORCE in the force-override debug log — added reactively during review but never folded back into the description.
Suggestion: Update the PR Summary to list all four hardening changes actually present in the diff before merge, so reviewers/future readers see the true scope.
waynesun09 pointed out that pre-code.src.sh was sourcing the entire
post-failure-report.lib.sh (347 lines: gh comment posting, PUSH_TOKEN/
GH_TOKEN/PEM redaction, post_fail_to_issue/report_post_failure_to_issue)
just to reuse gha_echo/sanitize_gha_log_output. That grew the bundled
pre-code.sh from 230 to 584 lines and put comment-posting helpers in
scope at the most trust-sensitive point in the pipeline (runs before
sandbox creation).
Pull gha_echo, sanitize_gha_log_output, print_sanitized_gha_log,
sanitize_comment_workflow_commands, and _sanitize_workflow_value into a
new scripts/lib/gha-log-sanitize.lib.sh with no gh/comment-posting
logic. post-failure-report.lib.sh now sources that lib instead of
defining these itself, and pre-code.src.sh sources it directly instead
of the whole post-failure-report.lib.sh. Bundled pre-code.sh drops back
to 302 lines.
Also teaches bundle-sh.sh to strip a ${LIB_DIR}/ prefix the same way it
already strips ${SCRIPT_DIR}/ and ${SCRIPT_DIR_POST}/, since a library
sourcing a sibling library needs to resolve relative to its own
directory (via BASH_SOURCE) rather than the top-level script's
directory — the caller's SCRIPT_DIR/SCRIPT_DIR_POST convention isn't
reliable from inside a sourced lib (post-code.src.sh/post-fix.src.sh
use SCRIPT_DIR_POST, not SCRIPT_DIR, and referencing the wrong one
under set -u aborts the script with "unbound variable").
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
waynesun09 noted the injection-hardening regression tests all assert negative/absence cases only — nothing confirms the ::notice:: line still fires on the validation-success path after the reordering. A future edit that deleted the notice entirely (not just reordered it) would pass the suite undetected. Verified the new test fails when the notice line is removed, and passes against the current script. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 6:14 PM UTC · Completed 6:29 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 6:31 PM UTC · Completed 6:38 PM UTC |
…ce through it - Add defence-in-depth sanitization of the level parameter in gha_echo to strip :: sequences, future-proofing against non-literal callers - Route the "Found existing human PR" notice through gha_echo instead of raw echo for consistency with the PR's hardening goal - Rebuild bundled scripts to propagate the library change Addresses review feedback on #576
🔧 Fix agent — iteration 2 (bot-triggered)Fixed 2 of 4 findings: added defence-in-depth sanitization of the gha_echo level parameter and routed the remaining raw ::notice:: through gha_echo. Disagreed with 2 findings: protected-path is a process concern requiring human approval, and the edge-case diagnostic fidelity tradeoff was acknowledged by the reviewer as inherent. All pre-code tests pass. Bundled scripts rebuilt. Fixed (2):
Disagreed (2):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 6:39 PM UTC · Completed 6:56 PM UTC |
|
🤖 Finished Fix · ✅ Success · Started 6:58 PM UTC · Completed 7:02 PM UTC |
🔧 Fix agent — iteration 3 (bot-triggered)Both review findings are non-actionable by the fix agent. The protected-path finding is a governance policy requiring human approval (outside fix agent scope). The edge-case finding is an acknowledged security-over-debuggability tradeoff already covered by a regression test. All previous code-level findings (CODE_FORCE sanitization, gha_echo level parameter, notice routing) were addressed in earlier iterations. No code changes made. Disagreed (2):
Tests: passedNext steps:
|
|
@waynesun09 on the sibling scripts — filed #598 before this review came in to track |
_sanitize_workflow_value stripped literal "::" before stripping the "%0A"/"%0D" text tokens, so a value like ":%0A:notice:%0A:PWNED" had no "::" substring until the percent tokens were removed — at which point the flanking colons collapsed back into a live "::notice::" workflow command, defeating the injection hardening this PR set out to add. Loop the substitutions to a fixed point so no strip order can reconstruct a stripped token, and also collapse real embedded CR/LF bytes, since GHA's runner parses stdout line-by-line and a literal newline would start a new physical line the "::" check never sees. Adds regression tests in post-failure-report-test.sh reproducing both the percent-token reconstruction and the newline+reconstruction case. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep: 2 findings on the CR/LF and sibling-function gaps in the sanitizer, both verified reproducible against the current head commit.
| done | ||
| # GHA's runner parses stdout line-by-line; a literal CR/LF byte would start | ||
| # a new physical line the "::" strip above never sees. | ||
| value="${value//$'\r'/}" |
There was a problem hiding this comment.
[CRITICAL] _sanitize_workflow_value strips CR/LF only once, after the fixed-point loop, letting a raw CR byte reconstruct a live :: command
The fixed-point loop above (lines 27-35) correctly re-strips ::/%0A/%0a/%0D/%0d text tokens until convergence, closing the prior strip-order bypass. But the literal CR-byte strip on this line and the LF-to-space substitution on the next line run exactly once, after the loop has already exited, and are never re-checked. A value that uses a real CR byte (not the %0D text token) to separate colons produces no :: and no percent-token substring, so the loop sees zero changes and breaks immediately; the trailing one-shot CR strip then collapses the flanking colons into a live workflow command with no further pass to catch it.
I verified this against the current head commit by sourcing the file directly:
input=$':\r:notice:\r:PWNED-INJECTED-VIA-CR'
sanitize_gha_log_output "$input" # -> "::notice::PWNED-INJECTED-VIA-CR"
This is reachable through every caller of sanitize_gha_log_output/gha_echo/print_sanitized_gha_log, including the pre/post-script sinks this PR set out to close.
Suggestion: move the CR/LF strip lines inside the while true; ... done loop, before the prev/value equality check, so a :: reassembled by CR/LF removal triggers another iteration and gets caught by the token strip. Add a regression test using a literal $'\r' byte between colons, mirroring the existing %0A-reconstruction test.
|
|
||
| # Neutralize line-start GHA workflow commands in comment bodies without | ||
| # stripping mid-string :: (e.g. std::string in compiler output). | ||
| sanitize_comment_workflow_commands() { |
There was a problem hiding this comment.
[MEDIUM] sanitize_comment_workflow_commands (sibling function) still has the same percent-token reconstruction bug the fixed-point loop above was added to fix
_sanitize_workflow_value was hardened with a fixed-point loop specifically because "removing one token can reassemble another" (comment above, lines 23-26). This sibling function does a single non-looped pass — one leading-::cmd:: sed strip, then a one-shot %0A/%0a/%0D/%0d removal — and wasn't updated. Verified against the current head:
sanitize_comment_workflow_commands ':%0A:notice:%0A:PWNED-INJECTED' # -> "::notice::PWNED-INJECTED"
because the leading-command sed check runs before there's any literal :: to match, then the percent-strip collapses the flanking colons afterward with no re-check.
I traced the only call site (sanitize_failure_detail in post-failure-report.lib.sh) and its output goes only to gh issue comment/gh pr comment, never to actual GHA runner stdout — so this doesn't become a live, GHA-parsed workflow command (hence MEDIUM rather than the sibling's CRITICAL). The impact is a forged-looking ::notice::/::error::-prefixed line inside an otherwise-trusted CI-failure comment on a public issue/PR — content spoofing rather than command injection.
Separately, the leading-command allow-list regex (^::(warning|error|notice|debug|group|endgroup)::) omits other real GHA workflow commands (stop-commands, add-mask, save-state, set-output, set-env, add-path, echo), so a value starting with e.g. ::add-mask:: wouldn't even hit the sed strip.
Suggestion: apply the same fixed-point-loop pattern to the token stripping here (keeping the distinct leading-::cmd::-only first pass, re-run after token removal converges), and broaden the allow-list regex to a generic ^::[A-Za-z-]+:: prefix match. Add a regression test mirroring the sibling's %0A-reconstruction test.
|
Just dropping this for now. I need to focus on other issues. |
|
🤖 Finished Retro · ✅ Success · Started 4:34 PM UTC · Completed 4:49 PM UTC Commit: |
Retro: PR #576 — pre-code GHA injection hardeningThis security-hardening PR went through 5 review/fix cycles over 2 days (Jul 30 – Jul 31) before being closed unmerged on Aug 10. The author (ralphbean) dropped it citing the need to focus on other work. Two CRITICAL sanitizer bypass vulnerabilities identified by human reviewer waynesun09 on Aug 6 remained unaddressed. Review quality gapThe review agent's security sub-agent found only LOW-severity issues across all 5 review passes (output-consistency, edge-case diagnostic fidelity, CODE_FORCE injection, gha_echo level param). The human reviewer found 7 distinct findings including 2 CRITICAL bypass vulnerabilities:
The review agent found 0 of these 7 findings. The security sub-agent's existing prompt covers GHA workflow command injection but did not reason about substitution-chain interactions or adversarially test the sanitizer implementation. Rework rateOf 5 fix agent runs, 3 produced zero code changes — the fix agent correctly identified all remaining findings as governance concerns (protected-path) or acknowledged tradeoffs (edge-case) it couldn't resolve. These 3 runs consumed approximately 15 minutes of compute with no value. The edge-case finding (diagnostic fidelity when Evidence for existing open issues
No new proposalsAll improvement opportunities identified in this retro are already tracked by open issues. The existing issues, if implemented, would address the review quality gap (agents#282, #535), the loop friction (fullsend#1068, agents#106, #680), and the adjacent-code blind spot (agents#634). |
2 participants
Summary
::notice::forGITHUB_ISSUE_URLafter input validation so unvalidated values never reach GHA workflow command parsingCOMMENT_BODYecho with body length to avoid user-controlled content hitting stdout where GHA scans for::commands::error::lines and theCODE_FORCEforce-override debug log throughgha_echo, which sanitizes::and CR/LF sequences so already-known-invalid or attacker-controlled input can't inject a second workflow commandgha_echo's ownlevelparameterscripts/lib/gha-log-sanitize.lib.shsopre-code.src.shdoesn't have to source the fullpost-failure-report.lib.sh(noghcalls, no comment-posting logic) just to reuse themSplit out from #567 per review feedback — these security-hardening changes are unrelated to the TRIAGE_AUTO_CODE feature.
The same injection class exists in
pre-review.shandpre-fix.sh; tracked separately in #598 since it's outside this PR's scope.Test plan
make testpassesscripts/pre-code.src.shpre-code.shmatches source🤖 Generated with Claude Code