Skip to content

fix(security): move expression interpolations to env context in helm-update-chart#191

Merged
bedatty merged 4 commits intodevelopfrom
fix/codeql-injection-helm-update-chart
Apr 1, 2026
Merged

fix(security): move expression interpolations to env context in helm-update-chart#191
bedatty merged 4 commits intodevelopfrom
fix/codeql-injection-helm-update-chart

Conversation

@bedatty
Copy link
Copy Markdown
Contributor

@bedatty bedatty commented Mar 31, 2026

Lerian

GitHub Actions Shared Workflows


Description

Mitigates CodeQL-reported code injection vulnerabilities in helm-update-chart.yml by moving all ${{ }} expression interpolations out of run: blocks and into step-level env: blocks.

Findings addressed:

  • actions/code-injection/medium at line 197 — ${{ inputs.scripts_path }} interpolated directly in shell
  • actions/untrusted-checkout/medium at line 155 — mitigated by ensuring no untrusted expressions flow into shell execution

Steps hardened:

  • Build scripts, Process all components, Update README matrix, Commit changes, Create feature branch, Push branch and create PR, Summary, Send Slack notification
  • Secrets (SLACK_BOT_TOKEN_HELM) also moved from inline run: references to env: context

Type of Change

  • feat: New workflow or new input/output/step in an existing workflow
  • fix: Bug fix in a workflow (incorrect behavior, broken step, wrong condition)
  • perf: Performance improvement (e.g. caching, parallelism, reduced steps)
  • refactor: Internal restructuring with no behavior change
  • docs: Documentation only (README, docs/, inline comments)
  • ci: Changes to self-CI (workflows under .github/workflows/ that run on this repo)
  • chore: Dependency bumps, config updates, maintenance
  • test: Adding or updating tests
  • BREAKING CHANGE: Callers must update their configuration after this PR

Breaking Changes

None. All inputs, outputs, and behavior remain identical. Only the mechanism for passing values into shell scripts changed (from inline interpolation to environment variables).

Testing

  • YAML syntax validated locally
  • Triggered a real workflow run on a caller repository using @develop or the beta tag
  • Verified all existing inputs still work with default values
  • Confirmed no secrets or tokens are printed in logs
  • Checked that unrelated workflows are not affected

Caller repo / workflow run: N/A — security hardening only, no functional change

Related Issues

Closes #

Summary by CodeRabbit

  • Chores
    • Consolidated CI workflow environment handling and simplified shell steps for clearer, more reliable execution.
    • Reworked iteration and scripting patterns to avoid subtle shell parsing and subshell issues.
    • Updated Slack notification steps to use injected tokens and environment values, improving credential handling and notification reliability.

@bedatty bedatty requested a review from a team as a code owner March 31, 2026 21:57
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 31, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 74a075e7-f84d-49bb-b343-03cc28ad6d5d

📥 Commits

Reviewing files that changed from the base of the PR and between 084d3f5 and c3a4960.

📒 Files selected for processing (1)
  • .github/workflows/helm-update-chart.yml

Walkthrough

Step-level env vars moved most ${{ ... }} expressions into the Helm update workflow; in-script assignments removed. Shell loops converted to safe while IFS= read -r ...; do ...; done < <(...) patterns. Slack calls now use Authorization: Bearer ${SLACK_BOT_TOKEN} sourced from secrets.

Changes

Cohort / File(s) Summary
Workflow env, shell control flow, and Slack auth
/.github/workflows/helm-update-chart.yml
Exported GitHub expressions to step-level env (branch, paths, script flags, updated components, Slack context). Replaced for ... in $(...) and piped loops with while IFS= read -r ...; do ...; done < <(...). Removed in-script construction of workflow/mention vars. Slack requests now use SLACK_BOT_TOKEN and Severino payload built via jq. Executable invocation quoted ("./${SCRIPTS_PATH}/update-readme-matrix").

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately reflects the main change: moving expression interpolations to env context for security hardening in helm-update-chart.
Description check ✅ Passed Description follows template structure with all critical sections completed: objectives, type of change marked, breaking changes addressed, testing steps checked, and related issues noted.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/codeql-injection-helm-update-chart

Comment @coderabbitai help to get the list of available commands and usage tips.

@lerian-studio lerian-studio added size/S PR changes 50–199 lines workflow Changes to one or more reusable workflow files labels Mar 31, 2026
@lerian-studio
Copy link
Copy Markdown

lerian-studio commented Mar 31, 2026

🔍 Lint Analysis

Check Files Scanned Status
YAML Lint 1 file(s) ✅ success
Action Lint 1 file(s) ✅ success
Pinned Actions 1 file(s) ✅ success
Markdown Link Check no changes ⏭️ skipped
Spelling Check 1 file(s) ✅ success
Shell Check 1 file(s) ✅ success
README Check 1 file(s) ✅ success
Composite Schema no changes ⏭️ skipped

🔍 View full scan logs

@lerian-studio
Copy link
Copy Markdown

lerian-studio commented Mar 31, 2026

🛡️ CodeQL Analysis Results

Languages analyzed: actions

Found 1 issue(s): 1 Medium

Severity Rule File Message
🟡 Medium actions/untrusted-checkout/medium .github/workflows/helm-update-chart.yml:155 Potential unsafe checkout of untrusted pull request on privileged workflow.

🔍 View full scan logs | 🛡️ Security tab

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.github/workflows/helm-update-chart.yml (1)

380-380: ⚠️ Potential issue | 🟡 Minor

Fix unquoted ${SCRIPTS_PATH} — pipeline failure (SC2086).

The variable ${SCRIPTS_PATH} is not quoted, causing the shellcheck SC2086 failure in CI. Quote it to prevent globbing and word splitting.

Proposed fix
-            ./${SCRIPTS_PATH}/update-readme-matrix \
+            ./"${SCRIPTS_PATH}"/update-readme-matrix \
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/helm-update-chart.yml at line 380, The shell invocation
uses an unquoted variable in "./${SCRIPTS_PATH}/update-readme-matrix" causing
SC2086; update the command to quote the variable so it becomes
"./\"${SCRIPTS_PATH}\"/update-readme-matrix" (or simply
"./${SCRIPTS_PATH}/update-readme-matrix" with the variable wrapped in double
quotes) to prevent word splitting and globbing—locate the line containing ./
${SCRIPTS_PATH}/update-readme-matrix and wrap ${SCRIPTS_PATH} in double quotes.
🤖 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/helm-update-chart.yml:
- Around line 623-630: The Severino Slack payload is built via unsafe shell
interpolation into JSON (SEVERINO_TEXT -> SEVERINO_RESPONSE) which can break for
quotes/backslashes; change the curl --data construction to use jq to build the
JSON safely (similar to SLACK_PAYLOAD): use jq -n with --arg to set the channel
and text (referencing BOT_MENTION, SEVERINO_TEXT, SLACK_CHANNEL) to produce the
JSON string and pass that to curl, keeping the Authorization header using
SLACK_BOT_TOKEN and sending the jq output as the request body for
SEVERINO_RESPONSE.

---

Outside diff comments:
In @.github/workflows/helm-update-chart.yml:
- Line 380: The shell invocation uses an unquoted variable in
"./${SCRIPTS_PATH}/update-readme-matrix" causing SC2086; update the command to
quote the variable so it becomes "./\"${SCRIPTS_PATH}\"/update-readme-matrix"
(or simply "./${SCRIPTS_PATH}/update-readme-matrix" with the variable wrapped in
double quotes) to prevent word splitting and globbing—locate the line containing
./ ${SCRIPTS_PATH}/update-readme-matrix and wrap ${SCRIPTS_PATH} in double
quotes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2761160d-85da-4da3-8e4c-4d0b485c63f6

📥 Commits

Reviewing files that changed from the base of the PR and between 4c7e91a and 67d5f72.

📒 Files selected for processing (1)
  • .github/workflows/helm-update-chart.yml

@bedatty bedatty merged commit f42d549 into develop Apr 1, 2026
16 checks passed
@github-actions github-actions bot deleted the fix/codeql-injection-helm-update-chart branch April 1, 2026 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S PR changes 50–199 lines workflow Changes to one or more reusable workflow files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants