fix(ci): store regex in variables to fix notify-parent parse error - #8
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughCentralized payload handling was added to the GitHub Actions workflow: a write_output helper and new environment variables (EVENT_NAME, PAYLOAD_*, GH_REPOSITORY, GH_REPO_NAME, GH_ACTOR) replace multiple direct echo writes; regex patterns for change types were extracted into named variables and used in both branches. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/notify-parent.yml (1)
36-36:⚠️ Potential issue | 🟠 MajorPre-existing risk: potential command injection via
client_payload.message.The
${{ github.event.client_payload.message }}expression is directly interpolated into the shell script. If a malicious actor can trigger arepository_dispatchevent with a crafted message containing shell metacharacters (e.g.,$(malicious_command)or backticks), arbitrary commands could execute.Consider using an environment variable to safely pass the value:
🛡️ Suggested safer approach
- name: Determine source id: source + env: + PAYLOAD_MESSAGE: ${{ github.event.client_payload.message }} + PAYLOAD_REPO: ${{ github.event.client_payload.repo }} + PAYLOAD_REPO_NAME: ${{ github.event.client_payload.repo_name }} + PAYLOAD_SHA: ${{ github.event.client_payload.sha }} + PAYLOAD_SHORT_SHA: ${{ github.event.client_payload.short_sha }} + PAYLOAD_TYPE: ${{ github.event.client_payload.type }} + PAYLOAD_ACTOR: ${{ github.event.client_payload.actor }} run: | if [ "${{ github.event_name }}" = "repository_dispatch" ]; then - echo "repo=${{ github.event.client_payload.repo }}" >> $GITHUB_OUTPUT - echo "repo_name=${{ github.event.client_payload.repo_name }}" >> $GITHUB_OUTPUT - echo "sha=${{ github.event.client_payload.sha }}" >> $GITHUB_OUTPUT - echo "short_sha=${{ github.event.client_payload.short_sha }}" >> $GITHUB_OUTPUT - echo "message=${{ github.event.client_payload.message }}" >> $GITHUB_OUTPUT - echo "type=${{ github.event.client_payload.type }}" >> $GITHUB_OUTPUT - echo "actor=${{ github.event.client_payload.actor }}" >> $GITHUB_OUTPUT + echo "repo=$PAYLOAD_REPO" >> $GITHUB_OUTPUT + echo "repo_name=$PAYLOAD_REPO_NAME" >> $GITHUB_OUTPUT + echo "sha=$PAYLOAD_SHA" >> $GITHUB_OUTPUT + echo "short_sha=$PAYLOAD_SHORT_SHA" >> $GITHUB_OUTPUT + echo "message=$PAYLOAD_MESSAGE" >> $GITHUB_OUTPUT + echo "type=$PAYLOAD_TYPE" >> $GITHUB_OUTPUT + echo "actor=$PAYLOAD_ACTOR" >> $GITHUB_OUTPUTThis prevents shell interpretation of the payload contents since environment variables are not subject to shell expansion when referenced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/notify-parent.yml at line 36, The echo line directly interpolates github.event.client_payload.message allowing shell metacharacter injection; instead capture the payload into a dedicated environment variable (using the workflow's env/context or set-env style safely) and then write that variable to GITHUB_OUTPUT without re-evaluating it, e.g., assign github.event.client_payload.message to a safe variable and reference the variable when appending to $GITHUB_OUTPUT (avoid direct ${ { github.event.client_payload.message } } in the shell command).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/notify-parent.yml:
- Line 36: The echo line directly interpolates
github.event.client_payload.message allowing shell metacharacter injection;
instead capture the payload into a dedicated environment variable (using the
workflow's env/context or set-env style safely) and then write that variable to
GITHUB_OUTPUT without re-evaluating it, e.g., assign
github.event.client_payload.message to a safe variable and reference the
variable when appending to $GITHUB_OUTPUT (avoid direct ${ {
github.event.client_payload.message } } in the shell command).
4e218e3 to
37558de
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/notify-parent.yml:
- Around line 43-49: The write_output function uses a fixed delimiter "__EOF__"
which allows output injection/truncation; change write_output to generate a
per-call random delimiter (e.g., using a short random token from
openssl/sha1/LC_CTYPE + date or mktemp -u), check/regenerate if the random
delimiter appears inside the value, then use that delimiter variable in the
here-doc writes to GITHUB_OUTPUT (use the same variable name in the printf lines
and the closing marker) so each call uses a unique delimiter and cannot be
prematurely terminated by the value content; update the function name
write_output and its use sites accordingly.
Bash interprets `(` in `[:(]` character classes as an unmatched subshell opener, causing "unexpected EOF while looking for matching ')'" (exit code 2). Storing the regex in a variable avoids this because bash passes variable content directly to the regex engine without shell parsing. Resolves [[incidents/notify-parent-broken]] Implements [[tasks/meta-64]] Co-authored-by: Claude <claude@anthropic.com>
37558de to
2e0c281
Compare
Summary
[[ =~ ]]tests(in[:(]character class was interpreted as unmatched subshell openerunexpected EOF while looking for matching ')'(exit code 2)Root cause
Context
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit