fix(server): add shell=True to npm subprocess calls for Windows compa…#3194
fix(server): add shell=True to npm subprocess calls for Windows compa…#3194oguz-work wants to merge 3 commits into
Conversation
…tibility fix(server): add shell=True to npm subprocess calls for Windows compatibility
There was a problem hiding this comment.
Pull Request Overview
This PR fixes Windows compatibility issues with npm subprocess calls by adding the shell=True parameter to all npm-related subprocess invocations in the server.py file.
- Adds
shell=Trueparameter to npm subprocess calls to resolve Windows FileNotFoundError issues - Maintains backward compatibility with Unix-like systems where the parameter is harmless
- Ensures cross-platform functionality for npm command execution
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| async def start_vue_dev_server(): | ||
| await asyncio.create_subprocess_shell( | ||
| "npm run dev", stdout=sys.stdout, stderr=sys.stderr, cwd=MAGMA_PATH | ||
| "npm run dev", stdout=sys.stdout, stderr=sys.stderr, cwd=MAGMA_PATH, shell=True |
There was a problem hiding this comment.
Using shell=True with asyncio.create_subprocess_shell is redundant since create_subprocess_shell already executes commands through the shell by default. Consider using asyncio.create_subprocess_exec with the command split into arguments for better security and consistency with the other subprocess calls.
| "npm run dev", stdout=sys.stdout, stderr=sys.stderr, cwd=MAGMA_PATH, shell=True | |
| "npm run dev", stdout=sys.stdout, stderr=sys.stderr, cwd=MAGMA_PATH |
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True, shell=True) | ||
| logging.info("VueJS front-end build complete.") | ||
| app_svc.application.on_response_prepare.append(enable_cors) | ||
|
|
||
| if args.build: | ||
| if len(os.listdir(MAGMA_PATH)) > 0: | ||
| logging.info("Building VueJS front-end.") | ||
| subprocess.run(["npm", "install"], cwd=MAGMA_PATH, check=True) | ||
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True) | ||
| subprocess.run(["npm", "install"], cwd=MAGMA_PATH, check=True, shell=True) | ||
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True, shell=True) |
There was a problem hiding this comment.
Using shell=True with a list of arguments can lead to unexpected behavior and potential security issues. When using shell=True, the command should be passed as a string, not a list. Either change to string format or remove shell=True and handle Windows compatibility differently.
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True, shell=True) | ||
| logging.info("VueJS front-end build complete.") | ||
| app_svc.application.on_response_prepare.append(enable_cors) | ||
|
|
||
| if args.build: | ||
| if len(os.listdir(MAGMA_PATH)) > 0: | ||
| logging.info("Building VueJS front-end.") | ||
| subprocess.run(["npm", "install"], cwd=MAGMA_PATH, check=True) | ||
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True) | ||
| subprocess.run(["npm", "install"], cwd=MAGMA_PATH, check=True, shell=True) | ||
| subprocess.run(["npm", "run", "build"], cwd=MAGMA_PATH, check=True, shell=True) |
There was a problem hiding this comment.
Using shell=True with a list of arguments can lead to unexpected behavior and potential security issues. When using shell=True, the command should be passed as a string, not a list. Either change to string format or remove shell=True and handle Windows compatibility differently.
|
|
Thanks for identifying the Windows compatibility issue! However, The root cause is that on Windows, import shutil
npm = shutil.which("npm")
subprocess.run([npm, "run", "build"], cwd=MAGMA_PATH, check=True)This resolves to |
deacon-mp
left a comment
deacon-mp
left a comment
There was a problem hiding this comment.
Heads up — there's an alternative PR for the same Windows-npm problem at #3340 that doesn't add shell=True. It uses shutil.which('npm') to resolve npm.cmd on Windows and passes the resolved absolute path as argv[0] to subprocess.run / create_subprocess_exec. That's the standard "subprocess + Windows + npm" pattern and avoids:
- The Bandit B602 finding (
shell=Trueon subprocess). - The Windows quoting being implementation-defined when
shell=Trueis passed alongside a list-form argv. - The forward-compatibility risk of normalising
shell=Trueinserver.py— even though the args here are all constants today, the next refactor that interpolates an env var or a path becomes injectable.
Suggest closing this in favour of #3340 (already approved-shape and tested on Win) so we don't carry the smell.
…tibility
fix(server): add shell=True to npm subprocess calls for Windows compatibility
Description
Description:
This pull request introduces a small but essential fix to ensure that the Caldera server can successfully invoke npm commands on Windows environments.
📌 Problem
On Windows, subprocess.Popen() or subprocess.run() calls that execute npm commands without shell=True fail with a FileNotFoundError, even when npm is available in the system's PATH. This is due to how Windows handles executable resolution without a shell context.
🔧 Changes Made
Added shell=True to all subprocess calls that include npm in server.py.
No logic or behavior change on Unix-like systems (Linux/macOS), where this argument is harmless.
✅ Tested On
✅ Windows 10/11: npm commands now execute correctly
✅ Linux (Ubuntu 22.04): no side effects observed
🛡️ Impact
This change improves cross-platform compatibility, especially for Windows users who want to contribute to or extend Caldera without having to patch the code manually.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: