ossf / package-manager-best-practices

Collection of security best practices for package managers.
Apache License 2.0
159 stars 16 forks source link

RC npm: "Shrinkwrap.json" does not exist, and is hostile in a published package #10

Closed ljharb closed 2 years ago

ljharb commented 2 years ago

Perhaps you're thinking of npm-shrinkwrap.json.

A shrinkwrap file is very user-hostile to include in a published package, because it actively prevents deduping, and blocks consumers from updating through security vulnerabilities and bugfixes - it actively worsens security.

Only apps should have lockfiles (dev-only lockfiles like package-lock.json are a bit more nuanced, but are safe wrt this concern), and we should be very clear about that so nobody inadvertently thinks it's less than a terrible idea to ship a shrinkwrap file.

lirantal commented 2 years ago

@ljharb what's your take on using a shrinkwrap file for CLI apps and "deliverables" like that? I find that doing that for my projects, which get installed with npm install rather than a repository clone, have helped me ensure that everyone use deterministic versions. I accept that it also requires me to maintain an up to date version for packages I ship to ensure they include security fixes.

ljharb commented 2 years ago

@lirantal i think that it's at least defensible for a CLI app to do it, but I still don't think the benefits outweigh the downsides, especially for security updates.

I'm not aware of any major CLI app that does so.

lirantal commented 2 years ago

Yes I can't think of any major CLIs that follow this either, actually. I seem to recall Express/Doug was doing that in the past for all of their packages but I think that changed.

For me at least, I can say that I value being able to ship and account for reproducing issues with 100% confidence that what I tested is exactly what is running on end-users machine. Unlike SaaS or the web, those CLI apps can be bundled into other package managers like brew or shipped via Docker containers, which is why I'd like to be able to control the versions shipped. I'd say the security updates, while important, might be irrelevant for CLI apps, so their importance greatly diminishes as if you were to use a specific library inside a web app.

laurentsimon commented 2 years ago

@ljharb can you explain more about the problems with security updates? With shrinkwrap, The CLI maintainer takes responsibility for the updates. If the CLI consumer has an app like dependabot installed and the CLI maintainer commits to keeping the dependencies up-to-date, the consumer will receive the security updates on each new CLI release. Is that correct?

soldair commented 2 years ago

Its safe to say the best practice from the community perspective is only lock files and all mention of shrinkwraps is generally scrubbed from the public consciousness.

It was used most prominently by Eran Hammers team at walmart labs https://www.npmjs.com/package/hapi https://cdn.jsdelivr.net/npm/hapi@18.1.0/npm-shrinkwrap.json and now rests with the deprecation message.

This version contains severe security issues and defects and should not be used! Please upgrade to the latest version of
@hapi/hapi or consider a commercial license (https://github.com/hapijs/hapi/issues/4114)

This ecosystem of packages stopped releasing hapi. moved to @hapijs/hapi and stopped using shrinkwrap.

ljharb commented 2 years ago

@laurentsimon the entire npm ecosystem is designed such that the maintainer does NOT have the sole responsibility for updates - that the end consumer has that control (just like on the web). It's intensely hostile to users to prevent them from doing security updates themselves.

lirantal commented 2 years ago

@soldair can you elaborate on your 1st bullet on why would it be unpredictable? the whole point of the shrinkwrap file is to lock the entire dependency tree for down-stream consumers.

On the whole concept of shrinkwraps - we can make parallels to the Java / Maven ecosystem where the pom.xml file specifies explicit versions of JARs (read: libraries). Then the maintainers of said JARs decide which other nested JARs to bundle inside them. This is effectively the same result, and I don't think developers in the Java community see this as a maintenance burden or deeply flawed issue. Would love to hear your insights on this.

EDIT: I'm interested in exploring the cases of shrinkwrap file usage patterns from the point of view of "shipped packages" such as stand-alone CLIs, or for example Electron-based desktop apps, where the usage behavior is significantly different than using them in the form of consumable libraries, or application projects.

soldair commented 2 years ago

I meant that it makes the npm installing algorithm depend on the contents of the tarball rather than exclusively the packument to produce an ideal tree. Shrinkwrap support was a major reason yarn was faster than npm when it gained popularity originally.

Your use case does seem fine. if your cli auto updates and you ship a new version for every vulnerability you could effectively keep your clis on "safe" deps.

lirantal commented 2 years ago

Ahh. What you call "ideal tree" I interpret as "install-time resolution of dependency versions", aka "non-deterministic versions" :-)

Thanks for sharing your perspective, appreciate that.

laurentsimon commented 2 years ago

@soldair what do you mean by stand-alone CLIs vs application projects?

lirantal commented 2 years ago

I'll attempt to do justice with Ryan's take for them:

laurentsimon commented 2 years ago

Thanks , that makes sense. Thanks everyone for the useful insights.

So the consensus is that shrinkwraps are acceptable for standalone CLI with the caveats highlighted in the discussion, but not for application projects or libraries. So we should make this clearer in the document - in particular, the doc does not currently differentiate between CLI and application projects.

Is this a fair reading of the discussion?

@ljharb what did you mean by dev-only lockfiles?

laurentsimon commented 2 years ago

answering my own question: dev lockfiles are those used by the maintainers (e.g., package-lock.json), but not by the consumers

erezrokah commented 2 years ago

Loving this discussion ❤️ Very insightful.

As someone who was until recently the lead maintainer of Netlify CLI, would like to add my vote 👍 for shrinkwrap (or any other locking mechanism) for CLIs. We added a shrinkwrap in https://github.com/netlify/cli/pull/2248, see https://github.com/netlify/cli/issues/2223 and https://github.com/netlify/cli/issues/2202 for reasoning.

We had numerous cases where a package we use broke SemVer and basically rendered the CLI useless (and often didn't even allow installation). Meaning our CLI would break without us changing anything on our end. Breaking SemVer can happen by mistake, but can also be intentional like in https://github.com/netlify/cli/issues/3981, see https://github.com/netlify/cli/issues/3981#issuecomment-1008368062.

For security issues/updates - 💯 to @lirantal that most of them are irrelevant for CLIs (e.g. regex denial of service issues). I don't recall we had a security incident for the 1+ year I was maintaining the CLI due to an out of date dependency.

Also, for dependencies updates, personally I'd rather use Renovate to manage those, since I can verify tests are passing after the update (we often caught breaking changes in patch/minor updates this way). We had Renovate set to auto merge for patch and minor versions, so assuming tests don't fail (and in that case we wanted to know about it), the maintenance toll is low.

We also recommend users to install the CLI as a local dev dependency and use a lock file in a CI environment.

wesleytodd commented 2 years ago

I think there is a missing nuance in this discussion:

Some CLI's are "applications" per @ljharb and @lirantal's definition above and some are not. What determines the split is how you call into it.

If you npm install it into an application or library and then call it via package scripts (or npx relying on the installed version, or whatever), that is not an application and should not have a shrinkwrap. If instead you always run it stand alone via npx or via a global install (which is bad for other reasons, so don't take this as me advocating for global installed packages) then it is in fact an application and can have a lock file if you want.

FWIW, most cli tools published to the registry are not applications and so that is why I also usually recommend not publishing using lock files. For example, eslint or tsc. But there are a small set of cases where it makes sense.

An example of a true "application" cli: We have a series of complicated build/deployment script we publish as packages. Those are called in custom built docker containers as entry scripts and are never installed into other libs or applications. To ensure reliable and reproducible builds we use a shrinkwrap, but that is an implementation detail which we could have achieved in other ways avoiding publishing.

stand-alone CLIs are CLI projects that get shipped or installed and their purpose is to be consumed as a locally installed program, rather than be interacted with over HTTP or as a service deployed somewhere.

So I think this is getting close, but not quite where I would draw the line.


I seem to recall Express/Doug was doing that in the past for all of their packages but I think that changed.

The express approach was specifically to deal with problems of the time, but it used non-range specifiers in package.json, not shirnkwraps. I do not recommend anyone else do what Express does. The key thing being that for "internal" deps (ones owned by the express project but published independently) it did not lock them. It is entirely a trust problem and driven by express' unique place in the ecosystem.

lirantal commented 2 years ago

If you npm install it into an application or library and then call it via package scripts (or npx relying on the installed version, or whatever), that is not an application

I feel that this is very subjective but I agree :-)

wesleytodd commented 2 years ago

Maybe there is a way to make the line more clear. I agree it is so fuzzy that I have tried to explain it multiple times and still I am not sure it is clear to people. Is it more clear to say:

If it ends up in another projects package.json or lockfile, it is not an application and should not have a shrinkwrap

laurentsimon commented 2 years ago

A few things I am missing:

@erezrokah said:

We also recommend users to install the CLI as a local dev dependency

Do you mean using the manifest's devDependencies or something else?

If it's via devDependencies, is it contradictory to @wesleytodd If it ends up in another projects package.json or lockfile, it is not an application and should not have a shrinkwrap?

My understanding is that it should depend on whether the manifest declares the CLI dependency (for npm install) OR just refer to it in scripts (maybe assuming the dependency is already installed on the machine or installing it as part of the script run...?). Is my understanding correct?

If the above is correct: how can the maintainer of a project control how their consumers install it? Is that why @wesleytodd said most cli tools published to the registry are not applications and so that is why I also usually recommend not publishing using lock files?

related question: Are devDependencies versions resolved independently from prod ones?

ljharb commented 2 years ago

If a project isn't meant to be installed, it has private: true, and can't be installed.

Published CLI tools are not deployed applications, and thus pretty much never include lockfiles.

dev deps and prod deps are resolved the same, but can be filtered against separately.

wesleytodd commented 2 years ago

Published CLI tools are not deployed applications, and thus pretty much never include lockfiles.

This is almost always true. The exception would be if a tool explicitly documents that it should never be installed in a project, which does not block users from doing it but also 🤷 what more can an author do. So an example would be if you used npm to host and resolve semver-ness, but didn't intend for the code to be installed in a project.

To be very clear, I have not seen this in practice in the OSS ecosystem, but have seen it in practice at multiple companies who publish internal JS libs.

So all in all I am comfortable if the "best practice" document really just says "if it is published it shouldn't use a lock", but as with many "best practices", the exception make the the rule. I think the way it is laid out in the new PR is good and accurate, so IMO it is good as is but if you wanted to simplify so novice end users are not confused about the distinction I would also be alright with that.

erezrokah commented 2 years ago

Do you mean using the manifest's devDependencies or something else?

Yes I mean as a dev dependency by running npm install --save-dev netlify-cli.

See https://docs.netlify.com/cli/get-started/#installation-in-a-ci-environment: image

The problem I want to solve as a CLI author (reasons described in https://github.com/ossf/package-manager-best-practices/issues/10#issuecomment-1153673448) is that every time a user runs npm install -g netlify-cli or npm install --save-dev netlify-cli they get the same exact CLI, unless a new version was published. Meaning when installing the CLI, locally or globally users always get the exact same version I, as an author, published. The same version I thoroughly tested.

Otherwise, the following can happen (and has happened multiple times):

  1. yarn global add netlify-cli works
  2. 1 minute passes without a CLI release
  3. yarn global add netlify-cli breaks and prints LIBERTY LIBERTY LIBERTY, see https://github.com/netlify/cli/issues/3981#issue-1097159039
lirantal commented 2 years ago

👆exactly

wesleytodd commented 2 years ago

@erezrokah Are we talking past each other or are we at an impasse? What I read from your comment, but is not directly said, is that you want to ship netflify-cli with a shirnkwrap. But that is directly counter to the point @ljharb and I were making above about things installed as dev deps should never use a lock file.

The way I read the linked issue (EDIT: I am well aware of this incident and it's impact, just realized what I wrote did not reflect my familiarity so wanted to clarify this was not just in response to reading your link) is that the tool needs to publish a version with a pin back, and existing apps need to keep their lock files. The alternative is apps never updating (even if they want to and there are security updates) because they will be locked to your response as a package author.

The goal is not to make the problem worse with a solution for a subset of the use cases. Additionally the end user should always come before the package maintainer in this. So taking the control away from the end user is bad unless the control was always in the wrong place and you also offer a way to "opt out" as the user. In this case there is neither a way to opt out (maybe overrides can, but that is unclear to me now) and also it is very clear maintainers do not do a better job of this than app owners in most cases.

erezrokah commented 2 years ago

Thanks @wesleytodd for adding more context.

I think I get your point now, please let me know if this makes sense:

  1. If installed locally never use a shrinkwrap file, as consumers can chose to pin dependencies via their own lock file AND get security updates, implicitly via SemVer ranges if not using a lock file, or explicitly by managing dependencies updates.

  2. If installed globally use use a shrinkwrap, otherwise tools can break unexpectedly with no way to fix them other than relying on the broken module author for a fix.

point @ljharb and I were making above about things installed as dev deps should never use a lock file.

Is this point valid to prod dependencies too? Also in some cases it's not clear if a package should be a dev or a prod dependency (especially when the output is bundled). See https://github.com/facebook/create-react-app/issues/10219 and https://stackoverflow.com/questions/44868453/create-react-app-install-devdepencies-in-dependencies-section/44872787#44872787. The docs https://nodejs.dev/learn/npm-dependencies-and-devdependencies only say what --save-dev does and not when to use it.

The alternative is apps never updating (even if they want to and there are security updates) because they will be locked to your response as a package author.

OK so I think I understand the tradeoff:

  1. Default to immutability of packages and lose implicit/controlled updates
  2. Default to implicit/controlled updates and lose immutability of packages

The goal is not to make the problem worse with a solution for a subset of the use cases. Additionally the end user should always come before the package maintainer in this. So taking the control away from the end user is bad unless the control was always in the wrong place and you also offer a way to "opt out" as the user. In this case there is neither a way to opt out (maybe overrides can, but that is unclear to me now) and also it is very clear maintainers do not do a better job of this than app owners in most cases.

💯 Completely agree on putting the users first. If there was a way to allow packages to be consumed in a predictable way and still let users update dependencies that would be best.

I've just stumbled upon https://research.swtch.com/vgo-intro#minimal_version_selection which is an interesting approach, that I don't completely understand yet, and planning to give it a deeper read.

jeffmendoza commented 2 years ago

Closing this as resolved with #25. Please take a look and if anyone has a concern we'll re-open and continue the discussion