doyensec / electronegativity

Electronegativity is a tool to identify misconfigurations and security anti-patterns in Electron applications.
Apache License 2.0
957 stars 65 forks source link

Don't publish npm-shrinkwrap.json #91

Open balgillo opened 3 years ago

balgillo commented 3 years ago

Describe the bug This project publishes its npm-shrinkwrap.json. That's discouraged:

It's strongly discouraged for library authors to publish this file, since that would prevent end users from having control over transitive dependency updates.

The practical impact is that electronegativity's dev dependencies are ending up in our package-lock.json (marked as either optional or extraneous, maybe depending on npm version). This is even though we are declaring electronegativity as a devDependency so its dev dependencies should be ignored. That may be caused by an npm issue. But if electronegativity didn't publish its npm-shrinkwrap.json, that bug wouldn't matter.

There are three dev dependencies that are particularly problematic because they or their dependencies have security advisories against them:

To Reproduce Steps to reproduce the behavior:

  1. npm init in new directory
  2. In package.json add:
    "devDependencies": {
    "@doyensec/electronegativity": "^1.9.0"
    }
  3. npm install --include=dev
  4. Search in package-lock.json for "base", "chokidar", "snapdragon-node" to see optional or extraneous dependencies.

Expected behavior Expect there to be no npm-shrinkwrap.json published with electronegativity, so its package.json is used to declare its dependencies. Only the runtime dependencies of electronegativity (and their trees of runtime dependencies) should end up in our package-lock.json.

Platform (please complete the following information):

ikkisoft commented 3 years ago

I am under the impression that npm-shrinkwrap.json is NPM's version of yarn.lock, isn't it?

If so, this is our best way to enforce the installation of particular dependencies and avoid all "works on my machine" issues See: https://classic.yarnpkg.com/blog/2016/11/24/lockfiles-for-all/

balgillo commented 3 years ago

@ikkisoft I think it's good to have this file in your repository, but not to publish it with your library. Unlike yarn.lock, npm-shrinkwrap.json does affect your consumers, via package-lock.json (see https://docs.npmjs.com/cli/v6/commands/npm-shrinkwrap#description)

tessro commented 3 years ago

We've been tracing an issue where npm seems to sporadically rewrite our package-lock.json. It seems to only happen to dependencies of Electronegativity. I suspect it's related to the use of shrinkwrap, since it isn't happening to any other libraries in our repo. Linking in case it turns out to be related: https://github.com/npm/cli/issues/3225

tessro commented 3 years ago

This has started causing a new problem for us as well, since npm-shrinkwrap.json includes things like fsevents v1.2.12, which causes install to fail on Windows. fsevents is a dependency of chokidar which is in shrinkwrap but not even used by electronegativity from what I can tell.

Unfortunately I think we are going to have to back out electronegativity from our testing pipeline until we can run it without pulling all these dependencies. Please consider dropping this file from the published package. 🙏🏼

tessro commented 3 years ago

@ikkisoft this bit from that Yarn blog post is the key:

When you install dependencies in your application or library, only your own yarn.lock file is respected.

As @balgillo mentions, this is different from NPM, which will respect the shrinkwrap file during install.

phosphore commented 3 years ago

Hello and thanks for reporting this!

Electronegativity was designed to be a command-line tool for auditors, which only later was adapted to be optionally used as a library. The documentation on NPM you cited also states:

The recommended use-case for npm-shrinkwrap.json is applications deployed through the publishing process on the registry: for example, daemons and command-line tools intended as global installs or devDependencies.

I'm convinced that the majority of our users are using it as a standalone, global install, but I'm willing to remove the shrinkwrap file for the time being and check with you if you're still experiencing problems (see v1.9.1). I reserve the right to restore it in case of reproducibility issues related to dependency discrepancies.

tessro commented 3 years ago

Understood!

Have you considered separating out the library from the CLI and publishing them as separate artifacts? That might solve the problem of publishing npm-shrinkwrap.json while letting people consume the library without conflicts.

bvandenbon commented 2 years ago

I haven't used electronegativity. But isn't this a cli tool ? And according to their documentation, it should be installed globally.

That means, you could just leave it out your package.json Then it shouldn't impact the dependencies of your specific project at all.