fix(lambda-nodejs): use direct spawn for local bundling#37292
fix(lambda-nodejs): use direct spawn for local bundling#37292mergify[bot] merged 7 commits intomainfrom
Conversation
### Issue #
Closes cdklabs/cdk-ops#4931.
### Reason for this change
The local bundling path builds a single shell command string and
executes it via `spawnSync("bash", ["-c", command])`. User-controlled
bundling properties are interpolated into this string without
sanitization. Using direct `spawnSync` with argument arrays is the
idiomatic Node.js approach and avoids shell interpretation entirely.
### Description of changes
Replace shell-based command execution in the local bundling path with
direct `spawnSync` calls using argument arrays.
- `PackageManager.runBinCommand()` returns `string[]` instead of a
joined string. Docker-path callers use `.join(" ")`.
- New `BundlingStep` discriminated union type: `shell` (commandHooks),
`spawn` (esbuild/tsc/install), `fs` (file operations).
- `createLocalBundlingSteps()` builds the step sequence for local
bundling using `toCliArgsArray()` and
`getTsconfigCompilerOptionsArray()` (no shell quoting needed).
- `tryBundle()` executes steps sequentially by type.
- Docker bundling path is completely unchanged.
- `commandHooks` remain shell-executed (user-provided by contract).
### Describe any new or updated permissions being added
N/A
### Description of how you validated changes
- All 125 existing aws-lambda-nodejs tests pass (6 suites).
- Updated `Local bundling` test to assert direct spawn calls.
- Added 6 new tests: esbuild options via spawn, nodeModules with
fs operations, commandHooks via shell, preCompilation with tsc
spawn, shell metacharacter handling, and pnpm workspace/cleanup.
- Full `lerna run build --scope=aws-cdk-lib` passes.
### Checklist
- [x] My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES
---
*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| case 'shell': | ||
| for (const cmd of step.commands) { | ||
| exec( | ||
| osPlatform === 'win32' ? 'cmd' : 'bash', |
There was a problem hiding this comment.
Technically speaking you should use process.env.COMSPEC as your shell on Windows.
There was a problem hiding this comment.
Added process.env.COMSPEC
| /** | ||
| * Creates structured bundling steps for local execution via direct spawn (no shell). | ||
| */ | ||
| private createLocalBundlingSteps( |
There was a problem hiding this comment.
This new function giatn is not matched by an equally giant function disappearing.
I would expect this to be a rewrite of something else?
There was a problem hiding this comment.
The local bundling steps are different than the one using docker. So both functions will remain, one will not replace the other.
There was a problem hiding this comment.
Oh. So we will have 2 functions like this:
function esBuildCommandForLocalExecution(): string[];
function esBuildCommandForDockerExecution(): string;With mostly the same logic, but a different return type?
If that's true, then next time we add an option or make a change, we need to do it in 2 places?
There was a problem hiding this comment.
I see, I'll deduplicate any common functions here.
There was a problem hiding this comment.
Refactored the functions with deduplication.
| exec( | ||
| osPlatform === 'win32' ? 'cmd' : 'bash', | ||
| [osPlatform === 'win32' ? '/c' : '-c', cmd], | ||
| { ...execOptions, windowsVerbatimArguments: osPlatform === 'win32' }, |
There was a problem hiding this comment.
cwd is part of execOptions.
There was a problem hiding this comment.
If so then it should also be part of execOptions in the "spawn" branch of the switch?
| ...(step.cwd ? { cwd: step.cwd } : {}), | ||
| }); | ||
| break; | ||
| case 'fs': |
There was a problem hiding this comment.
| case 'fs': | |
| case 'callback': |
?
b54e3bf to
b5e8509
Compare
b5e8509 to
d9444c0
Compare
- Use getTsconfigCompilerOptionsArray in Docker path instead of naive string splitting to properly handle compiler option values with spaces - Wrap callback step operations with try-catch for contextual error messages during local bundling file operations - Add regression tests for Docker preCompilation tsconfig handling - Add test for callback failure error context - Add getTsconfigCompilerOptionsArray parity tests in util.test.ts - Remove unused getTsconfigCompilerOptions import from bundling.ts
d9444c0 to
d84a5db
Compare
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status
This pull request spent 42 minutes 45 seconds in the queue, including 42 minutes 33 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Reason for this change
The local bundling path builds a single shell command string and executes it via
spawnSync("bash", ["-c", command]). User-controlled bundling properties are interpolated into this string without sanitization. Using directspawnSyncwith argument arrays is the idiomatic Node.js approach and avoids shell interpretation entirely.Description of changes
Replace shell-based command execution in the local bundling path with direct
spawnSynccalls using argument arrays.PackageManager.runBinCommand()returnsstring[]instead of a joined string. Docker-path callers use.join(" ").BundlingStepdiscriminated union type:shell(commandHooks),spawn(esbuild/tsc/install),fs(file operations).createLocalBundlingSteps()builds the step sequence for local bundling usingtoCliArgsArray()andgetTsconfigCompilerOptionsArray()(no shell quoting needed).tryBundle()executes steps sequentially by type.commandHooksremain shell-executed (user-provided by contract).Describe any new or updated permissions being added
N/A
Description of how you validated changes
Local bundlingtest to assert direct spawn calls.lerna run build --scope=aws-cdk-libpasses.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license