fix(tests): use string form for shell=True subprocess.run in nodejs test start scripts - #2245
Open
noron12234 wants to merge 1 commit into
Open
fix(tests): use string form for shell=True subprocess.run in nodejs test start scripts#2245noron12234 wants to merge 1 commit into
noron12234 wants to merge 1 commit into
Conversation
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two Windows-only start scripts under
tests/ten_runtime/integration/nodejs/standalone_test_nodejs_{2,3}/default_extension_nodejs/tests/bin/start.pypass a list argument together withshell=True:Python's
subprocessdocs discourage this combination. Concrete behavior differs by platform:/bin/sh -c npm install→ sh withcmd=\"npm\"and$0=\"install\"npmexecutes with no arguments — install is discarded, the step silently no-opscmd.exe /c \"npm install\"(vialist2cmdline)Same file, line 53 of
standalone_test_nodejs_3already uses the pattern for list arguments (subprocess.run([\"node\", \"--expose-gc\", \"build/index.js\"], env=env)— noshell=True), which is what makes theshell=Truein the other call sites read as the copy-paste anomaly.Fix
Switch the five affected calls to the documented string form for
shell=True:list2cmdline([\"npm\",\"install\"])already produces\"npm install\", so the actual cmdline passed tocmd.exe /cis byte-identical — zero behavior change on the platform this script is meant to run on.npm installinstead 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=Trueentirely becausenpmon Windows isnpm.cmd—CreateProcessdoesn't searchPATHEXT, sosubprocess.run([\"npm\", \"install\"])without a shell would fail on the target platform. String form +shell=Trueis the minimal-blast-radius fix that matches Python's own documentation.Follows the general subprocess hardening thread in #2240.