Skip to content
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

feat: add shell.cmd to replace exec #866

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@nfischer
Copy link
Member

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

feat: add shell.cmd to replace exec
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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

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.

src/cmd.js Outdated
});

function _cmd(options, command, commandArgs, userOptions) {
// `options` will usually no have a value: it's added by our commandline flag

This comment has been minimized.

Copy link
@freitagbr

freitagbr Jul 17, 2018

Contributor

usually not*

This comment has been minimized.

Copy link
@nfischer

nfischer Jul 17, 2018

Author Member

Done

@nfischer

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

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).

nfischer added some commits Jul 17, 2018

@fernandopasik

This comment has been minimized.

Copy link

commented Mar 6, 2019

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

@nfischer

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link

commented Jul 8, 2019

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

@caiomdias caiomdias referenced this pull request Jul 12, 2019

Merged

Solve security problems #231

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.