Skip to content

ci: fix expression injection in notify-on-review-wanted workflow #63720

Open
XananasX7 wants to merge 2 commits into
nodejs:mainfrom
XananasX7:fix/workflow-expression-injection-notify
Open

ci: fix expression injection in notify-on-review-wanted workflow #63720
XananasX7 wants to merge 2 commits into
nodejs:mainfrom
XananasX7:fix/workflow-expression-injection-notify

Conversation

@XananasX7
Copy link
Copy Markdown

The .github/workflows/notify-on-review-wanted.yml workflow injects GitHub Actions expressions directly into a shell run step. This is flagged by zizmor/actionlint and violates GitHub's security hardening guidelines. Fix: use env vars instead of inline expressions.

The workflow used GitHub Actions expressions directly inside shell run steps:

    if [[ -n "${{ github.event.pull_request.number }}" ]]; then
      number="${{ github.event.pull_request.number }}"
      number="${{ github.event.issue.number }}"

Fix: move expressions into env vars, reference env vars in shell.

Ref: https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections

Signed-off-by: El Mehdi Abenhazou <mehdiananas007@gmail.com>
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Jun 3, 2026
@Renegade334
Copy link
Copy Markdown
Member

These are non-user-controlled numeric variables, so shell escaping is not a concern here? We already pass string variables appropriately.

@XananasX7
Copy link
Copy Markdown
Author

That's a fair point — pull_request.number and issue.number are indeed integers and can't carry shell metacharacters. Strictly speaking, the risk here is lower than with string-typed inputs like titles or labels.

That said, I'd argue the env-var approach still has value even for numeric inputs:

  1. Consistency with existing code: the same step already wraps TITLE_ISSUE and TITLE_PR as env vars. Using a different pattern for the numbers makes the workflow harder to read and maintain.
  2. OSSF Scorecard / zizmor: both tools flag direct context interpolation in run: steps regardless of type, so this change keeps the workflow clean in automated scans.
  3. Forward safety: if the trigger inputs or the logic around this step ever changes, a string value injected here would be immediately safe rather than needing a separate fix.

Happy to close this if the team prefers to keep the existing style — just wanted to explain the rationale. Up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

meta Issues and PRs related to the general management of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants