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

Exec function calls JSON.stringify on command #772

Closed
beefancohen opened this issue Sep 18, 2017 · 3 comments
Closed

Exec function calls JSON.stringify on command #772

beefancohen opened this issue Sep 18, 2017 · 3 comments

Comments

@beefancohen
Copy link

beefancohen commented Sep 18, 2017

Node version (or tell us if you're using electron or some other framework):

v6.10.2

ShellJS version (the most recent version/Github branch you see the bug on):

0.7.8

Operating system:

Mac osx

Description of the bug:

Exec function calls JSON.stringify on the command. This causes issues when trying to pass 64 bit unsigned long strings as arguments since JSON.stringify cannot natively handle them. They get truncated and data is lost.

Is JSON.stringify necessary or can we add a flag to avoid parsing like that?

exec('foo --id 909845429478596609')
@freitagbr
Copy link
Contributor

freitagbr commented Sep 19, 2017

I think your problem may not be with JSON.stringify. In the case of exec, it's used to address escaping issues and code-injection when the temporary JS file is written. In your example:

exec('foo --id 909845429478596609')

ends up in the temporary JS file as:

var childProcess = child.exec("foo --id 909845429478596609", {}, function(err) {
    // ...
});

In other words, the individual parts of the command are not stringified, the whole command is. To illustrate how this prevents escaping and code-injection issues, take for example this command:

exec('echo"); console.log("vulnerable"); console.log("')

which ends up as:

var childProcess = child.exec("echo\"); console.log(\"vulnerable\"); console.log(\"", {}, function(err) {
    // ...
});

Without JSON.stringify, it might end up like this:

var childProcess = child.exec("echo"); console.log("vulnerable"); console.log("", {}, function(err) {
    // ...
});

Anyways, this was a long way of saying that the issue is probably with how foo is handling command-line arguments.

@nfischer
Copy link
Member

@evcohen what happens if you try child_process.execSync('foo --id 909845429478596609')? Success or failure?

@nfischer
Copy link
Member

We'll remove most uses of JSON.stringify() as part of #782. Therefore, closing this.

I suspect @freitagbr is right, the issue with foo is that it doesn't handle CLI arguments correctly.

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

No branches or pull requests

3 participants