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

Fixes vulnerability in shelljs #34

Closed
wants to merge 1 commit into from
Closed

Fixes vulnerability in shelljs #34

wants to merge 1 commit into from

Conversation

gl2748
Copy link

@gl2748 gl2748 commented Jan 22, 2018

Vulnerability:
shelljs/shelljs#143

@gl2748
Copy link
Author

gl2748 commented Jan 23, 2018

@kurttheviking thoughts on this?

@kurttheviking
Copy link
Owner

@gl2748 i havent had a chance to look yet; ill take a look soon. is this linked to a known cve?

@gl2748
Copy link
Author

gl2748 commented Jan 23, 2018

@kylemh
Copy link

kylemh commented Jan 23, 2018

Still a vulnerability, even in 8.1

@bnchdrff
Copy link

bnchdrff commented Feb 6, 2018

i don't think there is any security issue here. this library could just use child process exec commands and it'd be just as risky, with the downside of harder to read code.

see https://github.com/kurttheviking/git-rev-sync-js/blob/master/index.js and look for invocations of shell via _command - none use dynamic input values.

@kurttheviking
Copy link
Owner

@gl2748 @bnchdrff executing shell commands is inherently risky but this package doesn't allow user input to modify system calls in any way. as a result, I'm closing this as wontfix unless someone has a clear path forward to avoid system calls at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants