chore(security): prevent script and commit-message injections

[olivermeyer]

No description provided.

[Copilot]

Pull request overview

Hardens GitHub Actions workflows against script/command injection by avoiding direct interpolation of GitHub context/step-output expressions inside run: bash blocks, and instead passing values via env with proper quoting.

Changes:

• Refactors commit-message retrieval in ci-cd.yml to use env vars for event data and quotes commit messages safely.
• Updates release publishing steps to pass tag/changelog paths via env vars and quote them when used by cat/gh.
• Updates the composite test action to pass inputs via env vars and avoid direct expression interpolation in shell.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
.github/workflows/ci-cd.yml	Uses env vars for event fields and quotes commit message handling when exporting to outputs.
.github/workflows/_package-publish.yml	Uses env vars for changelog/tag/notes content and quotes CLI arguments for release creation and printing notes.
.github/actions/run-tests/action.yml	Uses env vars for composite-action inputs when running make and writing step summaries.

[claude]

Claude finished @olivermeyer's task in 4m 13s —— View job

---

PR Review: Security Hardening for GitHub Actions ✅

This PR implements comprehensive and correct security improvements to prevent script injection and commit-message injection attacks in GitHub Actions workflows.

---

Summary of Changes

4 files changed with systematic security hardening:

1. .github/workflows/ci-cd.yml - Commit message extraction
2. .github/workflows/_package-publish.yml - Release notes handling
3. .github/workflows/build-native-only.yml - Commit message and trigger checks
4. .github/actions/run-tests/action.yml - Composite action inputs

---

✅ Security Improvements Validated

Attack Vector Mitigated

The PR prevents script injection where malicious content in GitHub context expressions (commit messages, step outputs, etc.) could execute arbitrary commands when interpolated directly into shell run: blocks.

Example attack scenario (now prevented):

# ❌ VULNERABLE (before):
run: echo "${{ github.event.head_commit.message }}"
# If commit message is: `"; rm -rf / #`
# Shell executes: echo ""; rm -rf / #"

# ✅ SECURE (after):
env:
  COMMIT_MSG: ${{ github.event.head_commit.message }}
run: echo "$COMMIT_MSG"
# Shell variable expansion is safe - no command injection possible

Security Best Practices Applied

1. Environment Variables - All GitHub context expressions moved to env: block
2. Proper Quoting - Shell variables wrapped in double quotes ("$VAR")
3. Safe Output - Uses printf '%s\n' instead of echo (prevents -n/-e interpretation)
4. Heredoc for Multiline - Uses EOF delimiters for safe multiline output
5. Consistent Pattern - Applied uniformly across all affected files

---

📋 Detailed Review

1. ci-cd.yml (Lines 43-64)

# ✅ EXCELLENT
env:
  GITHUB_EVENT_NAME: ${{ github.event_name }}
  PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }}
  HEAD_COMMIT_MESSAGE: ${{ github.event.head_commit.message }}
run: |
  if [ "$GITHUB_EVENT_NAME" == "pull_request" ]; then
    COMMIT_MESSAGE=$(git log -1 --format=%B $PR_HEAD_SHA)  # ← Quoted in build-native-only.yml
  else
    COMMIT_MESSAGE="$HEAD_COMMIT_MESSAGE"
  fi
  {
    echo "commit_message<<EOF"
    printf "%s\n" "$COMMIT_MESSAGE"  # ← Safe output
    echo "EOF"
  } >> $GITHUB_OUTPUT

Note: In ci-cd.yml:53, $PR_HEAD_SHA is not quoted in the git log command, but this is OK because:

• It's a GitHub-provided SHA (40-character hex string)
• Cannot contain shell metacharacters
• However, build-native-only.yml:40 does quote it ("$PR_HEAD_SHA"), which is more defensive

Suggestion: For consistency, consider quoting it in ci-cd.yml too (not critical).

---

2. _package-publish.yml

2a. Print release notes (Lines 169-172):

# ✅ EXCELLENT
env:
  GIT_CLIFF_CHANGELOG: ${{ steps.git-cliff.outputs.changelog }}
run: cat "$GIT_CLIFF_CHANGELOG"

2b. Create GitHub release (Lines 206-213, 220-227):

# ✅ EXCELLENT
env:
  GH_REF_NAME: ${{ github.ref_name }}
  GIT_CLIFF_CHANGELOG: ${{ steps.git-cliff.outputs.changelog }}
run: |
  gh release create "$GH_REF_NAME" ./dist/* ./dist_native_zipped/* \
    --notes-file "$GIT_CLIFF_CHANGELOG"

2c. Convert release notes (Lines 242-252):

# ✅ EXCELLENT - Addresses Copilot's comment!
env:
  GIT_CLIFF_CONTENT: ${{ steps.git-cliff.outputs.content }}
run: |
  SLACK_RELEASE_NOTES=$(printf '%s\n' "$GIT_CLIFF_CONTENT" | \
    sed -E 's/\[([^]]+)\]\(([^)]+)\)/<\2|\1>/g' | \
    sed -E 's/\*\*([^*]+)\*\*/*\1*/g')

Note: Copilot suggested using printf instead of echo - already implemented! ✅

---

3. build-native-only.yml (Lines 33-51, 63-68)

# ✅ EXCELLENT - Same pattern as ci-cd.yml
env:
  GITHUB_EVENT_NAME: ${{ github.event_name }}
  PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }}
  HEAD_COMMIT_MESSAGE: ${{ github.event.head_commit.message }}
run: |
  if [ "$GITHUB_EVENT_NAME" == "pull_request" ]; then
    COMMIT_MESSAGE=$(git log -1 --format=%B "$PR_HEAD_SHA")  # ← Quoted (good!)
  else
    COMMIT_MESSAGE="$HEAD_COMMIT_MESSAGE"
  fi

Bonus improvement: Lines 63-68 also secure the workflow trigger check:

# ✅ EXCELLENT
env:
  COMMIT_HAS_MARKER: ${{ contains(..., 'build:native:only') }}
  PR_HAS_MARKER: ${{ contains(..., 'build:native:only') }}
run: |
  if [[ "$COMMIT_HAS_MARKER" == "true" ]] || \
     [[ "$PR_HAS_MARKER" == "true" ]]; then

---

4. run-tests/action.yml (Lines 33-38, 50)

# ✅ EXCELLENT
env:
  MAKE_TARGET: ${{ inputs.make-target }}
  SUMMARY_TITLE: ${{ inputs.summary-title }}
run: |
  make $MAKE_TARGET
  # ...
  echo "# $SUMMARY_TITLE" >> $GITHUB_STEP_SUMMARY

---

🔍 Additional Patterns to Consider (Non-Blocking)

While reviewing, I found similar patterns in other workflows that could benefit from the same treatment:

_scheduled-test-daily.yml

Line 260:

FAILED_TESTS="${{ steps.test-status.outputs.failed_tests }}"

Line 353:

echo "❌ Workflow failed due to test failures: ${{ steps.test-status.outputs.failed_tests }}"

Analysis: failed_tests is a custom step output that could contain test names from code. While unlikely to be malicious, applying the same env var pattern would be more defensive.

Suggested fix (optional):

env:
  FAILED_TESTS: ${{ steps.test-status.outputs.failed_tests }}
run: |
  echo "❌ Workflow failed due to test failures: $FAILED_TESTS"

_test.yml & _scheduled-test-daily.yml (outcome checks)

Pattern:

if [ "${{ steps.unit.outcome }}" == "failure" ]

Analysis: steps.*.outcome values are GitHub-controlled (always one of: success, failure, cancelled, skipped), so this is not a vulnerability. However, for completeness and consistency, these could also use env vars.

---

📚 Documentation Update (Suggestion)

Consider adding this security pattern to .github/CLAUDE.md as a best practice:

## Security: Preventing Script Injection in Workflows

**CRITICAL**: Never interpolate GitHub context expressions directly into shell `run:` blocks.

### ❌ Vulnerable Pattern
```yaml
run: echo "${{ github.event.head_commit.message }}"
run: cat "${{ steps.example.outputs.file }}"

✅ Secure Pattern

env:
  COMMIT_MSG: ${{ github.event.head_commit.message }}
  OUTPUT_FILE: ${{ steps.example.outputs.file }}
run: |
  echo "$COMMIT_MSG"
  cat "$OUTPUT_FILE"

Key principles:

1. Move all GitHub context to env: block
2. Quote shell variables: "$VAR"
3. Use printf '%s\n' instead of echo for user-controlled content
4. Use heredoc (EOF) for multiline outputs

This would help prevent future vulnerabilities and educate contributors.

---

### ✅ Syntax & Breaking Changes

- **YAML Syntax**: ✅ All files syntactically correct
- **Breaking Changes**: ✅ None detected - behavior preserved
- **Consistency**: ✅ Pattern applied uniformly
- **Quoting**: ✅ Proper throughout (minor inconsistency noted above)

---

### 🎯 Verdict

**APPROVED** ✅

This PR represents **excellent security engineering**:
- Systematic identification and remediation of injection vulnerabilities
- Correct implementation of GitHub Actions security best practices
- Clean commit history with descriptive messages
- No breaking changes or syntax errors

**Recommendations:**
1. **Merge as-is** - The fixes are correct and comprehensive for the critical paths
2. **Optional follow-up**: Apply the same pattern to `_scheduled-test-daily.yml` and `_test.yml` for completeness
3. **Document the pattern** in `.github/CLAUDE.md` to prevent future regressions

Excellent work on this security hardening! 🔒

---

### Commit Quality

✅ **Conventional commits** - All 4 commits follow `fix(ci):` format  
✅ **Clear scope** - Each commit addresses one workflow/action  
✅ **Atomic changes** - Easy to review and revert if needed

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[Copilot]

Quote $PR_HEAD_SHA in the git log call (as done in build-native-only.yml) to avoid word-splitting/globbing and keep the shell style consistent.

Suggested change

	COMMIT_MESSAGE=$(git log -1 --format=%B $PR_HEAD_SHA)
	COMMIT_MESSAGE=$(git log -1 --format=%B "$PR_HEAD_SHA")

[Copilot]

steps.slack-notes.outputs.content is later inserted into the Slack payload without surrounding quotes (line 262). Previously this step used toJSON(...), so the output was already a JSON-escaped string literal; after switching to printf of raw content, the output will contain unescaped characters/newlines, which will likely make the payload invalid (and can break the Slack announcement step). Consider JSON-encoding the converted notes before writing them to $GITHUB_OUTPUT, or update the payload to quote/escape the value consistently.

[arne-aignx]

@olivermeyer Is this a Markdown output or a JSON output?
As far as I know, it's markdown and we can ignore this comment

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.
see 5 files with indirect coverage changes

[arne-aignx]

LGTM.

I left one question, if this is answered the PR can be merged

[arne-aignx]

@olivermeyer Is this a Markdown output or a JSON output?
As far as I know, it's markdown and we can ignore this comment