Sitelet https://web.archive.org/web/20201123154049/https://github.com/shelljs/shelljs/issues/772
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
evcohen opened this issue Sep 18, 2017 · 3 comments
Closed

Exec function calls JSON.stringify on command #772

evcohen opened this issue Sep 18, 2017 · 3 comments

Comments

@evcohen
Copy link

@evcohen evcohen 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 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

@nfischer nfischer commented Sep 20, 2017

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

@nfischer
Copy link
Member

@nfischer nfischer commented Oct 26, 2017

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.

@nfischer nfischer closed this Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.