Fixes running building angular in a path with spaces #32784
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
654-654:executeCommandhas the same vulnerability. Apply the same fix to line 657.The
executeCommandmethod (line 657) uses the identical vulnerable pattern:execa([command, ...args].join(' '), { shell: true, ... }). Arguments with spaces passed to this method will be incorrectly split by the shell.Fix by passing args as a separate array parameter to execa instead of joining them:
const execaProcess = execa(command, args, { cwd: cwd ?? this.cwd, stdio: stdio ?? 'pipe', encoding: 'utf8', shell: false, cleanup: true, env: { ...COMMON_ENV_VARS, ...env, }, ...execaOptions, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/common/js-package-manager/JsPackageManager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/common/js-package-manager/JsPackageManager.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/common/js-package-manager/JsPackageManager.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/common/js-package-manager/JsPackageManager.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/common/js-package-manager/JsPackageManager.ts
| // `execaSync` shares the same node.js limitation of spawnSync: if `shell: true` is used, the command filename and arguments | ||
| // must be quoted: https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows | ||
| // `execa` doesn't seems to offer an helper to do so, so manually quote arguments with spaces |
There was a problem hiding this comment.
Comment references Windows but issue #32778 is macOS.
The comment mentions "spawning bat and cmd files on Windows" and links to Windows-specific Node.js documentation, but the linked issue #32778 reports the failure on macOS. While the underlying shell quoting principle applies to both platforms, the comment may confuse readers about the scope of the fix.
Consider clarifying:
- // `execaSync` shares the same node.js limitation of spawnSync: if `shell: true` is used, the command filename and arguments
- // must be quoted: https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows
- // `execa` doesn't seems to offer an helper to do so, so manually quote arguments with spaces
+ // When using `shell: true`, arguments containing spaces must be quoted or the shell will split them.
+ // Manually quote arguments with spaces to handle paths with spaces (fixes #32778).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // `execaSync` shares the same node.js limitation of spawnSync: if `shell: true` is used, the command filename and arguments | |
| // must be quoted: https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows | |
| // `execa` doesn't seems to offer an helper to do so, so manually quote arguments with spaces | |
| // When using `shell: true`, arguments containing spaces must be quoted or the shell will split them. | |
| // Manually quote arguments with spaces to handle paths with spaces (fixes #32778). |
🤖 Prompt for AI Agents
In code/core/src/common/js-package-manager/JsPackageManager.ts around lines 608
to 610, the inline comment calls out Windows-specific behavior and links Windows
docs while the referenced issue (#32778) is macOS; update the comment to clarify
that the shell-quoting requirement applies across platforms (including macOS and
Windows) when using shell:true and that we manually quote arguments with spaces
to work around shell quoting inconsistencies, and optionally mention the macOS
issue number for context so readers aren’t confused about the scope of the fix.
| // `execaSync` shares the same node.js limitation of spawnSync: if `shell: true` is used, the command filename and arguments | ||
| // must be quoted: https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows | ||
| // `execa` doesn't seems to offer an helper to do so, so manually quote arguments with spaces | ||
| const commandResult = execaCommandSync([command, ...args].map(c => c.includes(' ') ? `"${c}"` : c).join(' '), { |
There was a problem hiding this comment.
Prefer execaSync with separate arguments over manual shell quoting.
The current approach of manually quoting arguments that contain spaces is fragile and has several limitations:
- Incomplete escaping: Only handles spaces; arguments containing quotes, backslashes,
$, backticks, or other shell metacharacters will break or create injection risks. - Platform differences: Shell quoting rules differ between Unix shells and Windows cmd.exe. Double quotes behave differently, and escape sequences vary.
- Security concern: An argument like
foo" && malicious_command && "barwould break out of the quotes.
Recommended solution: Use execaSync with separate command and arguments instead of execaCommandSync with a manually joined string. This delegates proper shell escaping to execa:
- const commandResult = execaCommandSync([command, ...args].map(c => c.includes(' ') ? `"${c}"` : c).join(' '), {
+ const commandResult = execaSync(command, args, {
cwd: cwd ?? this.cwd,
stdio: stdio ?? 'pipe',
shell: true,
cleanup: true,
env: {
...COMMON_ENV_VARS,
...env,
},
...execaOptions,
});This approach:
- Lets execa handle platform-specific escaping correctly
- Eliminates the manual join/quote logic
- Works consistently across macOS, Linux, and Windows
- Reduces injection risk
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // `execaSync` shares the same node.js limitation of spawnSync: if `shell: true` is used, the command filename and arguments | |
| // must be quoted: https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows | |
| // `execa` doesn't seems to offer an helper to do so, so manually quote arguments with spaces | |
| const commandResult = execaCommandSync([command, ...args].map(c => c.includes(' ') ? `"${c}"` : c).join(' '), { | |
| const commandResult = execaSync(command, args, { | |
| cwd: cwd ?? this.cwd, | |
| stdio: stdio ?? 'pipe', | |
| shell: true, | |
| cleanup: true, | |
| env: { | |
| ...COMMON_ENV_VARS, | |
| ...env, | |
| }, | |
| ...execaOptions, | |
| }); |
There was a problem hiding this comment.
Doesn't work, tested with current and latest execa, it is a mere wrapper around spawnSync that requires proper escaping.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
execa execaSync shell escaping behavior with shell true
💡 Result:
Short answer
- execa/execaSync will automatically escape arguments when you call execa(file, args) (no shell). That makes it safe to pass arbitrary strings as the args array. [1][2]
- When you set options.shell: true execa runs a shell and builds/executes a command string, so shell semantics apply and you no longer get the same safe argument-passing guarantee. execa switches to behavior where the command is executed through the shell (and on Windows it sets windowsVerbatimArguments). Use execa.command()/execaCommand() or pass a single command string if you need shell features, but be careful to properly quote/escape for the target shell. [2][3]
Notes / pointers - The docs say “Arguments are automatically escaped. They can contain any character, including spaces” for the non-shell API. [2]
- The docs also explain “no shell interpreter … is used unless the shell option is set” and document windowsVerbatimArguments (set when shell:true). That means with shell:true you must handle shell escaping/quoting for your platform/shell. [2][4]
If you want, tell me your platform and an example command and I’ll show the exact escaping/quoting to use.
Sources
[1] execa README / API (older notes about escaping).
[2] execa package documentation / API (arguments escaped; shell behavior; windowsVerbatimArguments).
[3] execa README — Shell and execaCommand/execaCommandSync guidance.
[4] execa GitHub / issues (notes about Windows/shell behavior).
@lmartorella Thank you for testing and correcting me! You're absolutely right. I looked into execa's documentation and confirmed that when shell: true is set, execa runs the command through a shell and does not automatically handle argument escaping—that responsibility falls to the caller.
With shell: true, my suggested approach of using execaSync(command, args, ...) would indeed still require manual escaping, just as you've implemented.
A few questions to help find the best path forward:
-
Is
shell: truerequired here? If not, you could useexecaSync(command, args, { shell: false, ... })(or omit the shell option since it defaults to false), and execa would then automatically handle all escaping safely, including spaces and other special characters. -
If shell features are needed, the current approach of quoting arguments with spaces is a good start for the immediate issue (paths with spaces). However, as you continue working with this, you might want to consider a more robust escaping function that handles other shell metacharacters (quotes, backslashes,
$, etc.) in a platform-aware way.
Since you've tested this and the current implementation solves the immediate issue (#32778), I'll defer to your judgment on the approach. Thanks for the clarification!
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 18 | 18 | 0 |
| Self size | 1.88 MB | 2.07 MB | 🚨 +190 KB 🚨 |
| Dependency size | 10.21 MB | 10.21 MB | 🚨 +45 B 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 43 | 43 | 0 |
| Self size | 22.93 MB | 30.19 MB | 🚨 +7.26 MB 🚨 |
| Dependency size | 17.36 MB | 17.36 MB | 🚨 +6 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 532 | 532 | 0 |
| Self size | 749 KB | 950 KB | 🚨 +202 KB 🚨 |
| Dependency size | 58.50 MB | 58.77 MB | 🚨 +267 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 3.83 MB | 4.10 MB | 🚨 +267 KB 🚨 |
| Dependency size | 21.54 MB | 21.80 MB | 🚨 +263 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 157 | 157 | 0 |
| Self size | 31 KB | 31 KB | 🎉 -9 B 🎉 |
| Dependency size | 22.92 MB | 23.19 MB | 🚨 +267 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 114 | 114 | 0 |
| Self size | 37 KB | 37 KB | 🚨 +12 B 🚨 |
| Dependency size | 19.48 MB | 19.75 MB | 🚨 +267 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 272 | 272 | 0 |
| Self size | 25 KB | 25 KB | 🎉 -3 B 🎉 |
| Dependency size | 43.46 MB | 43.72 MB | 🚨 +267 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 928 KB | 927 KB | 🎉 -1 KB 🎉 |
| Dependency size | 72.88 MB | 80.14 MB | 🚨 +7.26 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 169 | 169 | 0 |
| Self size | 35 KB | 35 KB | 🚨 +6 B 🚨 |
| Dependency size | 69.31 MB | 76.57 MB | 🚨 +7.26 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 44 | 44 | 0 |
| Self size | 1.55 MB | 1.55 MB | 🎉 -408 B 🎉 |
| Dependency size | 40.29 MB | 47.55 MB | 🚨 +7.26 MB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 627 KB | 894 KB | 🚨 +267 KB 🚨 |
| Dependency size | 28 KB | 28 KB | 🚨 +27 B 🚨 |
| Bundle Size Analyzer | Link | Link |
2 participants
Closes #32778
What I did
I supported the execution of scripts with arguments with spaces.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
No current test is available for
JsPackageManageraroundexecuteCommand.Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Checkout the sample described in https://storybook.js.org/tutorials/intro-to-storybook/angular/en/get-started/ on a folder with spaces. Tested on MacOS.
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Bug Fixes