Skip to content

fix(tools): use execFile instead of exec to avoid shell injection#5197

Draft
marcoscaceres wants to merge 9 commits intomainfrom
alert-autofix-47
Draft

fix(tools): use execFile instead of exec to avoid shell injection#5197
marcoscaceres wants to merge 9 commits intomainfrom
alert-autofix-47

Conversation

@marcoscaceres
Copy link
Copy Markdown
Contributor

Potential fix for https://github.com/speced/respec/security/code-scanning/47

Use child_process.execFile() (or spawn) with an explicit executable and argument array, instead of exec() with a single interpolated shell command string.

Best minimal fix in this file:

  1. Replace exec import with execFile.
  2. Refactor commandRunner(program) so it tokenizes the static program once (e.g., "java -jar <path>"), then combines those tokens with parsed cmd tokens, and calls a new toExecFilePromise(file, args, options).
  3. Replace toExecPromise(cmd, ...) with toExecFilePromise(file, args, ...) that calls execFile(file, args, ...).
  4. Keep behavior (logging, timeout, output piping) equivalent.

This removes shell interpretation while preserving current functionality in tools/release.cjs.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

@marcoscaceres
Copy link
Copy Markdown
Contributor Author

@copilot see the CI error about the commit message. Can you fix that please?

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 14, 2026

@copilot see the CI error about the commit message. Can you fix that please?

Fixed in commits 593763c and c81e958.

Two issues were addressed:

  1. The commits now use conventional commit format (fix(tools): ...)
  2. The splitArgs function had a Prettier formatting violation that was causing the lint CI to fail — fixed that too

Note: the PR title still shows the original non-conventional text. Since I can't update the PR title directly, you may need to edit it manually to something like fix(tools): use execFile instead of exec to avoid shell injection so the "Check PR title" CI check passes.

@marcoscaceres marcoscaceres changed the title Potential fix for code scanning alert no. 47: Shell command built from environment values fix: code scanning alert no. 47: Shell command built from environment values Apr 14, 2026
@marcoscaceres marcoscaceres changed the title fix: code scanning alert no. 47: Shell command built from environment values fix(tools): use execFile instead of exec to avoid shell injection Apr 14, 2026
@marcoscaceres marcoscaceres marked this pull request as ready for review April 15, 2026 09:02
marcoscaceres and others added 4 commits April 17, 2026 17:52
…m environment values

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Agent-Logs-Url: https://github.com/speced/respec/sessions/cf64d910-0bb6-4b8e-8a7b-e6ebd7300833

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
Replace the hand-rolled splitArgs regex parser with direct array
construction. The regex failed on paths with spaces (e.g. vnu-jar
path) and was unnecessary complexity for static command strings.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the tools/release.cjs release script to avoid shell injection by switching from child_process.exec() (shell command string) to child_process.execFile() (explicit executable + argv array).

Changes:

  • Replace exec with execFile in tools/release.cjs.
  • Refactor commandRunner() to build an argv array and call toExecFilePromise() instead of interpolating a shell command string.
  • Update the HTML validator invocation to run java with -jar as explicit args.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tools/release.cjs Outdated
Copilot AI and others added 2 commits April 17, 2026 08:30
…h spaces

Agent-Logs-Url: https://github.com/speced/respec/sessions/5d94d008-6cad-43d1-89d6-8907cf192c39

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
Agent-Logs-Url: https://github.com/speced/respec/sessions/5d94d008-6cad-43d1-89d6-8907cf192c39

Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
@marcoscaceres
Copy link
Copy Markdown
Contributor Author

@copilot can you check ci and fix the issue?

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 17, 2026

@copilot can you check ci and fix the issue?

The CI failure is in Core — Highlight > highlights remote languages not bundled by default with ReSpec — a pre-existing flaky test that intermittently fails due to timing with a web worker that dynamically fetches a syntax highlighting language definition. My changes only touch tools/release.cjs. The same test passed on earlier runs of this PR (e.g. commit 3460e17) and on the main branch CI with the merged code. No action needed on our end.

marcoscaceres and others added 2 commits April 18, 2026 05:21
All call sites now pass arrays. The string split path was dead code
that could silently break on spaced arguments.
@marcoscaceres marcoscaceres marked this pull request as draft April 19, 2026 12:44
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.

3 participants