generated from amazon-archives/__template_Apache-2.0
-
Notifications
You must be signed in to change notification settings - Fork 72
feat: bypass shell if app is given as a string array #1046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
rix0rrr
wants to merge
10
commits into
main
Choose a base branch
from
huijbers/leave-argv
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+131
−61
Conversation
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
When executing CDK apps, users specify the command as a string. The only feasible interpretation of that is by executing the command line through a shell, either `bash` or `cmd.exe`. We are using shell execution on purpose: - It's used for tests - It's necessary on Windows to properly execute `.bat` and `.cmd` files - Since we have historically offered it you can bet dollars to doughnuts that customers have built workflows depending on that. This is all a preface to explain why we don't have an `argv` array. Automated code scanning tools will probably complain, but we can't change any of this. And since the source of the string and the machine it's executing on are part of the same security domain (it's all "the customer": the customer writes the command string, then executes it on their own machine), that is fine. However, historically we did do trivial parsing and preprocessing of that `string` in order to help the user achieve success. Specifically: if the string pointed to a `.js` file we would run that `.js` file through a Node interpreter, even if: - The file was not marked as executable on POSIX; or - There was no shell association set up for `.js` files on Windows. That light parsing used to fail in the following cases: - If the pointed-to file had spaces in its path. - If Node was installed in a location that had spaces in its path. In this PR we document the choice of command line string a bit better, and handle the cases where the file or interpreter paths can have spaces in them. We still don't do fully generic command line parsing, because it's extremely complex on Windows and we can probably not do it correctly; we're just concerned with quoting the target and interpreter. Closes #636
auto-merge was automatically disabled
January 21, 2026 13:50
Pull request was converted to draft
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.
When executing CDK apps, users specify the
{ "app" }command as:string(heavily advertised); orstring[](not really advertised but historically possible through specific code paths).Traditionally we would collapse both forms to a single string, by joining the array with spaces, and then shell executing them.
Security people don't like shell execution very much, because there are chances the command gets injected by special characters and redirected to do something else. Developers don't shell shell execution very much because they have to pay attention to quoting of paths with spaces in them.
If we have a
string[]already, we shouldn't need to run a command through the shell anymore, but we do. This PR changes the behavior as follows:This protects users against quoting issues and shell injections.
A potential breaking change of this PR could be that if people used to put shell strings in arrays, knowing full well they would execute through the shell regardless, that won't do what they expect anymore. Examples of cursedness that would fail if we merge this:
The risk profile of this "fix/feat" depends on how many instances of the above there are. Chances are good there will be 0 or negligibly many.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license