Skip to content

feat: add shell.cmd to replace exec #866

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 30, 2019
Merged

feat: add shell.cmd to replace exec #866

merged 1 commit into from
Oct 30, 2019

Conversation

nfischer
Copy link
Member

@nfischer nfischer commented Jul 3, 2018

This adds an initial implementation of shell.cmd(), which is intended as
the eventual replacement for shell.exec(). This PR does not fully
implement the API, but demonstrates a simple and secure alternative, and
will allow further iteration to cover other use cases in follow-up PRs.

Design doc: https://shelljs.page.link/cmd-design

Issue #495
Test: automated test suite

@nfischer nfischer added feature exec Issues specific to the shell.exec() API security labels Jul 3, 2018
@nfischer nfischer requested a review from freitagbr July 3, 2018 05:46
@nfischer nfischer mentioned this pull request Jul 3, 2018
Closed
@nfischer
Copy link
Member Author

nfischer commented Jul 3, 2018

Apparently newlines are always converted to \n instead of os.EOL. Fixed in the second commit.

Windows has a separate issue with executing shx. Still looking into the correct fix.

@nfischer
Copy link
Member Author

nfischer commented Jul 3, 2018

It turns out that Windows poses an extra issue. node_modules/.bin/ is populated with .bat files, but .bat and .cmd files require a shell. This is a major use case, so I think we have no choice but to enable the shell option on Windows and to resort to meta-character-escaping. I'll investigate and add a section to the doc.

This was referenced Jul 9, 2018
Open
Open
Closed
Closed
Closed
@nfischer nfischer added this to the v0.9.0 milestone Jul 12, 2018
freitagbr
freitagbr previously requested changes Jul 17, 2018
View reviewed changes
@nfischer
Copy link
Member Author

Currently investigating Windows behavior on branch dns-cmd-on-win (which is not intended to be submitted, but it's easier to experiment with appveyor than to experiment locally).

This was referenced Jul 26, 2018
Closed
Closed
@nfischer nfischer mentioned this pull request Jan 29, 2019
Closed
@fernandopasik
Copy link

would this fix the security issue described here? embarklabs/embark#1329

@silasbw silasbw mentioned this pull request May 20, 2019
Merged
@nfischer nfischer mentioned this pull request Jun 24, 2019
Closed
@nfischer
Copy link
Member Author

nfischer commented Jul 8, 2019

Hi all! I would like to clarify: this pull request adds a new feature to our module. It is not a security fix.

shell.exec() is working as intended, and it is the dependent module's responsibility to use this method securely. Please see our security guidelines, and feel free to respond to #945 (comment) if you have further questions regarding Snyk or Github/WhiteSource's vulnerability reports.

@fernandopasik
Copy link

Thank you for the clarification @nfischer and your hard work!

@nfischer nfischer force-pushed the simple-exec-alternative branch from 255ce33 to 9a27e96 Compare October 23, 2019 06:47
@codecov-io
Copy link

codecov-io commented Oct 23, 2019 •
edited
Loading

Codecov Report

Merging #866 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #866      +/-   ##
==========================================
+ Coverage   97.14%   97.22%   +0.07%     
==========================================
  Files          34       35       +1     
  Lines        1298     1332      +34     
==========================================
+ Hits         1261     1295      +34     
  Misses         37       37
Impacted Files Coverage Δ
commands.js 100% <ø> (ø) ⬆️
src/cmd.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05374a7...186412d. Read the comment docs.

@nfischer
feat: add shell.cmd to replace exec
186412d 
This adds an initial implementation of shell.cmd(), which is intended as
the eventual replacement for shell.exec(). This PR does not fully
implement the API, but demonstrates a simple and secure alternative, and
will allow further iteration to cover other use cases in follow-up PRs.

Design doc: https://shelljs.page.link/cmd-design

Issue #495
Test: automated test suite
@nfischer nfischer force-pushed the simple-exec-alternative branch from 9a27e96 to 186412d Compare October 29, 2019 08:09
@nfischer nfischer dismissed freitagbr’s stale review October 29, 2019 08:40

freitagbr is no longer active with the project

@nfischer nfischer merged commit a421b9e into master Oct 30, 2019
@nfischer nfischer deleted the simple-exec-alternative branch October 30, 2019 01:35
@conartist6 conartist6 mentioned this pull request Nov 18, 2020
Closed
nfischer added a commit that referenced this pull request Feb 20, 2025
@nfischer
feat: expose new shell.cmd()
daaedf1 
shell.cmd() was originally implemented in #866, however was not yet
exposed. This command should be ready for people to try out, so this
exposes the command by default.

This is still not a full implementation of everything in the
https://shelljs.page.link/cmd-design design doc, however this completes
the initial phase and is likely good enough to replace most use cases of
synchronous shell.exec().

Fixes #495
@nfischer nfischer mentioned this pull request Feb 20, 2025
Merged
nfischer added a commit that referenced this pull request Feb 20, 2025
@nfischer
feat: expose new shell.cmd()
c0fa2c0 
shell.cmd() was originally implemented in #866, however was not yet
exposed. This command should be ready for people to try out, so this
exposes the command by default.

This is still not a full implementation of everything in the
https://shelljs.page.link/cmd-design design doc, however this completes
the initial phase and is likely good enough to replace most use cases of
synchronous shell.exec().

Fixes #495
nfischer added a commit that referenced this pull request Feb 20, 2025
@nfischer
feat: expose new shell.cmd()
65baedb 
shell.cmd() was originally implemented in #866, however was not yet
exposed. This command should be ready for people to try out, so this
exposes the command by default.

This is still not a full implementation of everything in the
https://shelljs.page.link/cmd-design design doc, however this completes
the initial phase and is likely good enough to replace most use cases of
synchronous shell.exec().

Fixes #495
kmashint pushed a commit to kmashint/shelljs that referenced this pull request Apr 10, 2025
@nfischer @kmashint
feat: expose new shell.cmd()
85737f7 
shell.cmd() was originally implemented in shelljs#866, however was not yet
exposed. This command should be ready for people to try out, so this
exposes the command by default.

This is still not a full implementation of everything in the
https://shelljs.page.link/cmd-design design doc, however this completes
the initial phase and is likely good enough to replace most use cases of
synchronous shell.exec().

Fixes shelljs#495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freitagbr freitagbr freitagbr left review comments

Assignees
No one assigned
Labels
exec Issues specific to the shell.exec() API feature security
Projects
None yet
Milestone
v0.9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@nfischer @fernandopasik @codecov-io @freitagbr