harden: if unverified user data can reach the exec me... in...

[anupamme]

Summary

Harden input handling in Utilities/DataGenerator/convert-cli.js (flagged by semgrep).

Vulnerability

Field	Value
ID	javascript.shelljs.security.shelljs-exec-injection.shelljs-exec-injection
Severity	HIGH
Scanner	semgrep
Rule	javascript.shelljs.security.shelljs-exec-injection.shelljs-exec-injection
File	Utilities/DataGenerator/convert-cli.js:68
Assessment	Defensive hardening

Description: If unverified user data can reach the exec method it can result in Remote Code Execution

Threat Model Context

This is a Node.js library - vulnerabilities affect downstream consumers who use this package.

Changes

• Utilities/DataGenerator/convert-cli.js

Behavior Preservation

The change is scoped to 1 file on the vulnerable path; it only tightens handling of untrusted input and leaves valid inputs unaffected.

---

This patch removes an exploit primitive — a code pattern that, while not independently exploitable today, could be chained with other weaknesses by automated exploit-development tooling. Proactive removal of such primitives raises the bar against increasingly capable automated attack tools.

---

Automated security fix by OrbisAI Security

[finetjul]

Please fix commit name

[anupamme]

done. pls review.

[finetjul]

Why not extend the reasoning to shell.find ?

[anupamme]

shell.find() is worth considering because --paraview can influence the paths it receives, but I don’t think it has the same command-injection sink semantics as shell.exec(). find() performs filesystem traversal rather than passing a constructed string to a system shell. The actual injection primitive was the cmdLine.join(' ') → shell.exec() path, which is now replaced with execFileSync() and preserves argument boundaries.

That said, I’ll add a regression test covering shell metacharacters in --input/--output to make the security property explicit.

[anupamme]

Added a regression test (Utilities/DataGenerator/convert-cli.security-test.js, runnable via npm run test:convert-cli-security) that spawns convert-cli.js against a stub pvpython and asserts shell metacharacters passed via --input/--output are never shell-interpreted and reach pvpython as a literal argv element — making the shell.exec() → execFileSync() security property explicit, as promised.

While wiring this test up end-to-end, I found that the CLI's help-check condition (!options.help) was always true — commander's opts() never populates a help key, so program.outputHelp(); process.exit(0); fired on every invocation, making the execFileSync calls (including the ones this PR just patched) unreachable dead code. This predates this PR (introduced in a commander v9 migration back in 2021) but I fixed it in a separate commit so the new regression test could actually exercise the real, patched code path rather than testing the pattern in isolation.

[sankhesh]

Dropping the options.help is a behavior change, imho. @finetjul wdyt?

[anupamme]

This was intentional, not accidental — commander never populates opts().help for the built-in -h/--help flag (that flag is intercepted and the process exits during .parse(), before this code runs). So options.help was always undefined, and !options.help was always true, meaning the original condition printed help and exited unconditionally on every invocation regardless of arguments — the pvpython execFileSync branches were unreachable. This is fixed and explained in more detail in commit 73c8bac ("fix(DataGenerator): convert-cli no longer always prints help"), which is already part of this branch. Happy to add a comment in the code if that would help future readers, @finetjul let me know if you see it differently.

[finetjul]

Please fix linting error (too long commit message)