Skip to content

fix(tests): use string form for shell=True subprocess.run in nodejs test start scripts - #2245

Open
noron12234 wants to merge 1 commit into
TEN-framework:mainfrom
noron12234:fix/tests-nodejs-shell-true
Open

fix(tests): use string form for shell=True subprocess.run in nodejs test start scripts#2245
noron12234 wants to merge 1 commit into
TEN-framework:mainfrom
noron12234:fix/tests-nodejs-shell-true

Conversation

@noron12234

Copy link
Copy Markdown
Contributor

Problem

Two Windows-only start scripts under tests/ten_runtime/integration/nodejs/standalone_test_nodejs_{2,3}/default_extension_nodejs/tests/bin/start.py pass a list argument together with shell=True:

result = subprocess.run([\"npm\", \"install\"], env=env, shell=True)

Python's subprocess docs discourage this combination. Concrete behavior differs by platform:

platform what runs outcome
POSIX /bin/sh -c npm install → sh with cmd=\"npm\" and $0=\"install\" npm executes with no arguments — install is discarded, the step silently no-ops
Windows cmd.exe /c \"npm install\" (via list2cmdline) works — but only by accident of how Windows list2cmdline joins the list

Same file, line 53 of standalone_test_nodejs_3 already uses the pattern for list arguments (subprocess.run([\"node\", \"--expose-gc\", \"build/index.js\"], env=env) — no shell=True), which is what makes the shell=True in the other call sites read as the copy-paste anomaly.

Fix

Switch the five affected calls to the documented string form for shell=True:

result = subprocess.run(\"npm install\", env=env, shell=True)
  • Windows: list2cmdline([\"npm\",\"install\"]) already produces \"npm install\", so the actual cmdline passed to cmd.exe /c is byte-identical — zero behavior change on the platform this script is meant to run on.
  • POSIX: someone running the Python fallback from a Unix shell (e.g. a Mac dev debugging locally) now actually invokes npm install instead of silently no-op'ing.

Files changed

  • tests/ten_runtime/integration/nodejs/standalone_test_nodejs_2/default_extension_nodejs/tests/bin/start.py (3 call sites)
  • tests/ten_runtime/integration/nodejs/standalone_test_nodejs_3/default_extension_nodejs/tests/bin/start.py (2 call sites)

Why this shape

Not removing shell=True entirely because npm on Windows is npm.cmdCreateProcess doesn't search PATHEXT, so subprocess.run([\"npm\", \"install\"]) without a shell would fail on the target platform. String form + shell=True is the minimal-blast-radius fix that matches Python's own documentation.

Follows the general subprocess hardening thread in #2240.

…est start scripts

The Windows-only start scripts under
tests/ten_runtime/integration/nodejs/standalone_test_nodejs_{2,3} pass a list
argument together with shell=True:

    subprocess.run(["npm", "install"], env=env, shell=True)

Python's subprocess docs advise against this combination. On POSIX,
    /bin/sh -c npm install
runs sh with cmd="npm" and $0="install", so npm executes with no arguments
and silently no-ops. On Windows, subprocess joins the list via list2cmdline
before running through cmd.exe, so the call happens to work — but only by
accident of that particular platform's shell.

Switch to the documented string form for shell=True. Behavior is identical
on Windows (list2cmdline of ["npm","install"] is already "npm install") and
is correct on POSIX (someone running this Python fallback from a Unix shell
now actually invokes npm install).

Same file, line 53 of standalone_test_nodejs_3 already uses shell=False with
a list ('subprocess.run(["node", "--expose-gc", "build/index.js"], env=env)'),
which is the pattern for list arguments — so the shell=True in the other
call sites is the copy-paste anomaly.

Follows the general subprocess hardening thread in TEN-framework#2240.
@noron12234
noron12234 requested a review from halajohn as a code owner July 27, 2026 07:56
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.

1 participant