Fix: Avoid shell mangling during eslint --init #8936

andersk · 2017-07-12T19:55:12Z

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix (template)

Tell us about your environment

ESLint Version: 4.2.0
Node Version: 4.8.3
npm Version: 3.5.2

What did you do? Please include the actual source code causing the issue.

Ran eslint --init and selected the Google style guide.

What actually happened? Please include the actual, raw output from ESLint. See above.

This ended up calling execSync("npm i --save-dev eslint>=4.1.1"). Because execSync spawns the child through a shell, this had the effect of running npm i --save-dev eslint with its output redirected to a new file named =4.1.1, leaving the wrong version in package.json.

$ npm init -y
[…]
$ eslint --init
? How would you like to configure ESLint? Use a popular style guide
? Which style guide do you want to follow? Google
? What format do you want your config file to be in? JavaScript
Checking peerDependencies of eslint-config-google
Local ESLint installation not found.
Installing eslint-config-google@latest, eslint@>=4.1.1
npm WARN test@1.0.0 No description
npm WARN test@1.0.0 No repository field.
Successfully created .eslintrc.js file in /tmp/test
ESLint was installed locally. We recommend using this local copy instead of your globally-installed copy.
$ ls
=4.1.1  node_modules  package.json
$ cat package.json
{
  "name": "test",
  "version": "1.0.0",
  "description": "",
  "main": ".eslintrc.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "devDependencies": {
    "eslint": "^4.2.0",
    "eslint-config-google": "^0.9.1"
  }
}

It’s always better to avoid spawning a shell when possible for correctness reasons, to say nothing of scary security reasons.

What changes did you make? (Give an overview) Replaced all uses of execSync with execFileSync to avoid spawning a shell, and updated the corresponding tests.

Is there anything you'd like reviewers to focus on? Nothing in particular.

jsf-clabot · 2017-07-12T19:55:13Z

All committers have signed the CLA.

eslintbot · 2017-07-12T19:55:14Z

LGTM

not-an-aardvark · 2017-07-12T20:28:50Z

Thanks for the PR, this definitely seems like a bug.

Does this fix work on Windows (and if it does, does it work on Windows with Node 4)? I recall that it wasn't possible to use an array of arguments with execFile on some platforms. (If you're not able to test this, there are a few other people on the team who use Windows that can test it for you.)

If we can't pass the arguments directly then we could fall back to escaping them to solve the immediate issue.

It’s always better to avoid spawning a shell when possible for correctness reasons, to say nothing of scary security reasons.

Agreed. In this case, I don't think the problem is exploitable because crafting malicious input would require the attacker to put something malicious in the package on npm, and if they can do that then they can already run arbitrary code anyway (since we're downloading the package immediately afterwards). But I agree that it would be a good idea to avoid spawning a shell, if it's possible to do so with our supported platforms.

andersk · 2017-07-13T12:46:40Z

Argh. It does not work on Windows, and I can’t figure out a reasonable way to make it work on Windows, due to a combination of Windows insanity (Windows programs are launched with a single argument string, not an argument list; script files can only be launched through cmd.exe; and cmd.exe uses its own quoting rules that subtly different from the rest of the system) with Node insanity (you would think that there would be an API where you give it a list of strings and it runs a program in whatever way it has to such that it sees that list of strings, but actually, the APIs that look like they take a list of strings really just blindly smash them together with spaces).

The closest thing I can find to a solution is cross-spawn. Can we use that?

If you ran ‘eslint --init’ and selected the Google style guide, it would end up calling execSync("npm i --save-dev eslint@>=4.1.1"). Because execSync spawns the child through a shell, this had the effect of running ‘npm i --save-dev eslint@’ with its output redirected to a new file named ‘=4.1.1’, leaving the wrong version in package.json. Fix this by spawning processes using the cross-spawn package, which avoids spawning a shell at all on Unix and quotes the arguments appropriately on Windows. Signed-off-by: Anders Kaseorg <andersk@mit.edu>

eslintbot · 2017-07-13T13:24:56Z

LGTM

andersk · 2017-07-13T13:26:45Z

Here’s the fix using cross-spawn. I’ve tested this version to work on Ubuntu (Node.js 4.8.3) and Windows (Node.js 4.4.7 and Node.js 8.1.4).

not-an-aardvark · 2017-07-13T19:24:35Z

I'm not adamantly opposed to adding a dependency, but I would rather avoid it if there's a simple way to do this without a dependency.

Would it work to just use childProcess.spawnSync with an array of arguments?

andersk · 2017-07-13T20:49:30Z

@not-an-aardvark It would be entirely reasonable to assume that would work, but it doesn’t. On Windows,

childProcess.spawnSync('npm', ['i', '--save-dev', 'eslint@>=4.1.1']) fails with ENOENT.
childProcess.spawnSync('npm.cmd', ['i', '--save-dev', 'eslint@>=4.1.1']) runs the shell-mangled command, creating a file named 4.1.1.
childProcess.spawnSync('npm', ['i', '--save-dev', 'eslint@>=4.1.1'], { shell: true }) runs the shell-mangled command, creating a file named 4.1.1.

childProcess.execFileSync has the same behavior. I found the gory details at nodejs/node#7367.

not-an-aardvark · 2017-07-13T20:54:10Z

I think you're using spawnSync incorrectly. It should be

childProcess.spawnSync('npm', ['i', '--save-dev', 'eslint@>=4.1.1'])

without the first argument being in an array.

not-an-aardvark · 2017-07-13T21:19:01Z

As a separate note:

(edit: quote removed, issue has been addressed)

It's fine to criticize technical decisions that went into developing Node, but please refrain from personally attacking the people who develop Node. (See the "Be respectful" section in our code of conduct.)

andersk · 2017-07-13T21:30:00Z

I think you're using spawnSync incorrectly. It should be […]

@not-an-aardvark Right, that’s what I meant. I’ve fixed my comment above.

As a separate note: […]

Sorry, I should have channelled my frustration more productively. I have edited out that part of my comment above.

not-an-aardvark · 2017-07-13T21:44:09Z

Thanks, I appreciate it.

Do you know if there's a simple workaround (e.g. escaping the arguments) that would solve the issue despite the confusing Node API? My thought process is that this is a very minor part of ESLint that only happens in one place, and it would be nice if we didn't have to include an entire process-spawning module just for this one usage. (We generally try to avoid adding unnecessary dependencies because they increase the install time, among other reasons. But if the alternative would be to add a significant amount of complexity, then I'm fine with it.)

andersk · 2017-07-13T22:20:08Z

Depends on what you mean by “simple”. There are different, incompatible quoting rules for Windows (code in cross-spawn, which doesn’t need to quote on Unix) and Unix (code in shell-escape, which doesn’t support Windows). We could implement both of them behind a platform test. Or we could implement just Windows quoting in a wrapper that switches between exec on Windows and execFile on Unix.

not-an-aardvark · 2017-07-14T16:46:00Z

cc'ing @mysticatea since I think we might have run into a similar problem before with eslint-canary, but I'm not sure if there's a good solution.

binki · 2017-07-14T17:02:17Z

If possible, I’d recommend just using cross-spawn since they already probably solve most of these problems anyway. It’s a very commonly-depended-upon module so anything using eslint is likely already pulling it in through another dependency. If you implement your own shell escaping by basically copying cross-spawn’s code, then you have code duplication and it will get out of date…

mysticatea · 2017-07-15T04:18:42Z

Thank you for the carbon copy.

I think the best solution is {shell: true} option of cp.spawn() as I did on eslint-canary, but we cannot use it since Node 4 does not support. Alternatively, cross-spawn is a better solution, so I'm using it in npm-run-all as well.

However, in this case, I think enough that just enclosing by double quotes like npm show --json "${packageName}" peerDependencies because I believe the packageName never include double quotes. I'm OK with either way.

not-an-aardvark · 2017-07-15T04:32:47Z

For what it's worth, {shell: true} is supported starting in Node 4.8.0, but I don't think we can use it because we would need to support earlier versions (and we still support Node 5, which doesn't have it).

andersk · 2017-07-15T07:23:06Z

I obviously don’t get to decide which solution you accept, but speaking for myself, I’m not going to write a patch that merely concatenates double-quote characters around the argument, because I’d prefer not to be the author of code associated with future bugs and security vulnerabilities when the assumptions change or when it’s reused in a different context. Leaky abstractions are what got us into this mess; let’s avoid building more of them.

{shell: true} also doesn’t address the problem because it doesn’t escape the argument list (on Windows or Unix), even though the API makes it look like it should.

not-an-aardvark · 2017-07-15T07:28:47Z

Ok, you've convinced me that we should just use cross-spawn.

not-an-aardvark

LGTM. Thanks!

(edit: Waiting a day or two before merging this just in case anyone wants to review)

not-an-aardvark · 2017-07-16T03:57:16Z

Thanks for contributing!

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly cli Relates to ESLint's command-line interface labels Jul 12, 2017

andersk force-pushed the execFileSync branch from fdb3f74 to 12950a6 Compare July 13, 2017 13:24

not-an-aardvark approved these changes Jul 15, 2017

View reviewed changes

ilyavolodin approved these changes Jul 16, 2017

View reviewed changes

not-an-aardvark merged commit 55bc35d into eslint:master Jul 16, 2017

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Avoid shell mangling during eslint --init #8936

Fix: Avoid shell mangling during eslint --init #8936

andersk commented Jul 12, 2017

jsf-clabot commented Jul 12, 2017 •

edited

eslintbot commented Jul 12, 2017

not-an-aardvark commented Jul 12, 2017 •

edited

andersk commented Jul 13, 2017 •

edited

eslintbot commented Jul 13, 2017

andersk commented Jul 13, 2017

not-an-aardvark commented Jul 13, 2017

andersk commented Jul 13, 2017 •

edited

not-an-aardvark commented Jul 13, 2017

not-an-aardvark commented Jul 13, 2017 •

edited

andersk commented Jul 13, 2017 •

edited

not-an-aardvark commented Jul 13, 2017 •

edited

andersk commented Jul 13, 2017

not-an-aardvark commented Jul 14, 2017

binki commented Jul 14, 2017

mysticatea commented Jul 15, 2017 •

edited

not-an-aardvark commented Jul 15, 2017

andersk commented Jul 15, 2017

not-an-aardvark commented Jul 15, 2017

not-an-aardvark left a comment •

edited

not-an-aardvark commented Jul 16, 2017

Fix: Avoid shell mangling during eslint --init #8936

Fix: Avoid shell mangling during eslint --init #8936

Conversation

andersk commented Jul 12, 2017

jsf-clabot commented Jul 12, 2017 • edited

eslintbot commented Jul 12, 2017

not-an-aardvark commented Jul 12, 2017 • edited

andersk commented Jul 13, 2017 • edited

eslintbot commented Jul 13, 2017

andersk commented Jul 13, 2017

not-an-aardvark commented Jul 13, 2017

andersk commented Jul 13, 2017 • edited

not-an-aardvark commented Jul 13, 2017

not-an-aardvark commented Jul 13, 2017 • edited

andersk commented Jul 13, 2017 • edited

not-an-aardvark commented Jul 13, 2017 • edited

andersk commented Jul 13, 2017

not-an-aardvark commented Jul 14, 2017

binki commented Jul 14, 2017

mysticatea commented Jul 15, 2017 • edited

not-an-aardvark commented Jul 15, 2017

andersk commented Jul 15, 2017

not-an-aardvark commented Jul 15, 2017

not-an-aardvark left a comment • edited

Choose a reason for hiding this comment

not-an-aardvark commented Jul 16, 2017

jsf-clabot commented Jul 12, 2017 •

edited

not-an-aardvark commented Jul 12, 2017 •

edited

andersk commented Jul 13, 2017 •

edited

andersk commented Jul 13, 2017 •

edited

not-an-aardvark commented Jul 13, 2017 •

edited

andersk commented Jul 13, 2017 •

edited

not-an-aardvark commented Jul 13, 2017 •

edited

mysticatea commented Jul 15, 2017 •

edited

not-an-aardvark left a comment •

edited