Skip to content

fix(security): Replace execSync with execFileSync to prevent command injection#30

Open
fix-it-felix-sentry[bot] wants to merge 1 commit intomainfrom
fix/js-1502-command-injection-clang-format
Open

fix(security): Replace execSync with execFileSync to prevent command injection#30
fix-it-felix-sentry[bot] wants to merge 1 commit intomainfrom
fix/js-1502-command-injection-clang-format

Conversation

@fix-it-felix-sentry
Copy link

Description

This PR fixes a command injection vulnerability detected by Semgrep in the scripts/clang-format.mjs file.

Changes

  • Replaced child_process.execSync() with child_process.execFileSync() to prevent potential shell injection
  • Changed from string concatenation to passing arguments as an array for safer command execution
  • Removed the intermediate cmd variable that was constructing the shell command string

Why This Fix?

While the original code used hardcoded arguments, using execSync() with string concatenation is flagged as a security risk because:

  1. It invokes a shell unnecessarily, which can introduce vulnerabilities
  2. execFileSync() with an array of arguments is the recommended secure approach for executing commands
  3. This follows OWASP best practices for preventing command injection

Testing

The fix maintains the same functionality - it still executes the clang-format command with the same arguments, just in a safer manner.

References

…injection in clang-format script

- Replace child_process.execSync() with execFileSync() to prevent potential shell injection
- Use array of arguments instead of string concatenation for safer command execution
- Addresses command injection vulnerability flagged by Semgrep

Fixes: JS-1502
Related: VULN-1095

Co-Authored-By: fix-it-felix-sentry[bot] <260785270+fix-it-felix-sentry[bot]@users.noreply.github.com>
@linear
Copy link

linear bot commented Feb 18, 2026

@github-actions
Copy link

Semver Impact of This PR

🟢 Patch (bug fixes)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


Bug Fixes 🐛

  • (security) Replace execSync with execFileSync to prevent command injection by fix-it-felix-sentry[bot] in #30

Internal Changes 🔧

Release

  • Fix changelog-preview permissions by BYK in #29
  • Bump Craft version to fix issues by BYK in #27
  • Switch from action-prepare-release to Craft by BYK in #25

Other

  • Use pull_request_target for changelog preview by BYK in #28
  • macos-13 deprecation by timfish in #26

🤖 This preview updates automatically when you update the PR.


try {
child_process.execSync(cmd, {stdio: 'inherit'});
child_process.execFileSync('./node_modules/.bin/clang-format', args, {stdio: 'inherit'});
Copy link

Choose a reason for hiding this comment

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

Bug: The switch to execFileSync will cause the script to fail on Windows because it cannot execute the .cmd wrapper for clang-format without a shell.
Severity: HIGH

Suggested Fix

Add the { shell: true } option to the execFileSync call. This will allow it to correctly execute shell scripts like .cmd files on Windows. The updated call would be child_process.execFileSync('./node_modules/.bin/clang-format', args, {stdio: 'inherit', shell: true});.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: scripts/clang-format.mjs#L6

Potential issue: The change from `execSync` to `execFileSync` to run the `clang-format`
command will cause a runtime failure on Windows. On Windows, the executable in
`node_modules/.bin` is a `.cmd` batch file. `execFileSync` does not use a shell by
default and cannot execute `.cmd` files directly, unlike `execSync`. This will cause the
script to throw an error when `yarn lint` or `yarn fix:clang` is run in a Windows
environment, breaking the linting process for developers on that platform. This issue is
not caught by the current CI, which only runs the lint job on Ubuntu.

Did we get this right? 👍 / 👎 to inform future reviews.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants

Comments