Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Update recommendations for lockfiles #25

Merged
merged 28 commits into from Jul 13, 2022
Merged

Update recommendations for lockfiles #25

Changes from 20 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
c8a9d3b
Update lockfile description
laurentsimon Jun 28, 2022
f2e06a8
update
laurentsimon Jun 28, 2022
ed3eaf1
update
laurentsimon Jun 28, 2022
cc5c654
update
laurentsimon Jun 28, 2022
5d9d5fc
update
laurentsimon Jun 28, 2022
8b81f9c
Update review/npm.md
laurentsimon Jun 28, 2022
2fd0659
Update review/npm.md
laurentsimon Jun 28, 2022
ce3c62f
Update review/npm.md
laurentsimon Jun 28, 2022
a19a020
update
laurentsimon Jun 28, 2022
9cca608
Update review/npm.md
laurentsimon Jun 28, 2022
d014ad6
update
laurentsimon Jun 28, 2022
1db3592
update
laurentsimon Jun 28, 2022
46ec483
update
laurentsimon Jun 28, 2022
7908deb
Update review/npm.md
laurentsimon Jun 28, 2022
e8253af
Update review/npm.md
laurentsimon Jun 28, 2022
1ee1de4
update
laurentsimon Jun 28, 2022
6d8eb0e
update
laurentsimon Jun 28, 2022
238630e
Update review/npm.md
laurentsimon Jun 28, 2022
5bb7a87
comments
laurentsimon Jun 28, 2022
ecff088
update
laurentsimon Jun 28, 2022
575b376
update
laurentsimon Jul 1, 2022
0d5e822
update
laurentsimon Jul 1, 2022
893832d
update
laurentsimon Jul 1, 2022
044fc7f
update
laurentsimon Jul 1, 2022
425571e
update
laurentsimon Jul 1, 2022
a47c8be
update
laurentsimon Jul 1, 2022
7ce831f
update
laurentsimon Jul 1, 2022
42663ca
update
laurentsimon Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
94 changes: 71 additions & 23 deletions review/npm.md
View file
Open in desktop
Expand Up @@ -17,11 +17,12 @@ alternative.
* [Dependency management](#dependency-management)
+ [Intake](#intake)
+ [Declaration](#declaration)
+ [Project types](#project-types)
+ [Reproducible installation](#reproducible-installation)
- [Vendoring dependencies](#vendoring-dependencies)
- [Use a Lockfile](#use-a-lockfile)
* [Package-lock.json](#package-lockjson)
* [Shrinkwrap.json](#shrinkwrapjson)
* [npm-shrinkwrap.json](#shrinkwrapjson)
+ [Maintenance](#maintenance)
* [Release](#release)
+ [Account](#account)
Expand Down Expand Up @@ -94,6 +95,25 @@ commitish are allowed.
**Note**: The manifest file ***does not*** list transitive dependencies; it
lists only direct project dependencies.

### Project types

In the rest of this document, we will refer to three types of projects:

- **Libraries**: These are projects published on the npm registry and consumed
by other projects in the form of API calls. (Their manifest file
typically contains a `main`, `exports`, `browser`, `module`, and/or `types` entry).

- **Standalone CLIs**: These are projects published on the npm registry
and consumed by end-users in the form of locally installed programs that
are **always** run stand alone via `npx` or via a global install.
An example would be [clipboard-cli](https://github.com/sindresorhus/clipboard-cli).
(Their manifest file contains a `bin` entry).

- **Application projects**: These are projects that teams collaborate on in
development and deploy, such as web sites and/or web applications.
An example would be a React web app for a company's user-facing SaaS.
laurentsimon marked this conversation as resolved.
Show resolved Hide resolved
(Their manifest file typically contains `"private": true`).

### Reproducible installation

A reproducible installation is one that guarantees the exact same copy the
Expand Down Expand Up @@ -121,6 +141,11 @@ benefits, including:
proxy packages from public registries. Note: Versions are immutable in
principle, but immutability is enforced by the registry.

- Improve the accuracy of automated tools such as GitHub's security alerts.

- Let maintainers test updates before accepting them in the default branch,
e.g., via renovatebot's [stabilityDays](https://docs.renovatebot.com/configuration-options/#stabilitydays).

There are two ways to reliably achieve a reproducible installation: vendoring
dependencies and pinning by hash.

Expand Down Expand Up @@ -172,7 +197,7 @@ cryptographic hash of their content:
}
```

The package-lock.json file is a ***snapshot of an installation*** that allows
The `package-lock.json` file is a ***snapshot of an installation*** that allows
later reproduction of the same installation. As such, the lock file is
generated or updated via the various commands that install packages, e.g., `npm
install`. If some dependencies are missing or not pinned by hash (e.g.,
Expand All @@ -183,24 +208,24 @@ The lock file ***cannot*** be uploaded to a registry, which means that
consumers who locally install the package via `npm install` may [see different
dependency
versions](https://dev.to/saurabhdaware/but-what-the-hell-is-package-lock-json-b04)
than the repo owners used during testing. Using package-lock.json is akin to
than the repo owners used during testing. Using `package-lock.json` is akin to
dynamic linking for low-level programming languages: the loader will resolve
dependencies at load time using libraries available on the system. Using this
lock file leaves the task of deciding dependencies' versions to use to the
package consumer.

##### Shrinkwrap.json
##### npm-shrinkwrap.json

[Shrinkwrap.json](https://docs.npmjs.com/cli/v7/configuring-npm/package-lock-json#package-lockjson-vs-npm-shrinkwrapjson)
[npm-shrinkwrap.json](https://docs.npmjs.com/cli/v7/configuring-npm/package-lock-json#package-lockjson-vs-npm-shrinkwrapjson)
is another lock file supported by npm. The main difference is that this lock
file ***may*** be uploaded to a registry along with the package. This ensures that
consumers of the dependency will obtain the same dependencies' hashes as the
repo owners intended. With shrinkwrap.json, the package producer takes
repo owners intended. With `npm-shrinkwrap.json`, the package producer takes
responsibility for updating the dependencies on behalf of their consumers. It's
akin to static linking for low-level programming languages: everything is
declared at packaging time.

To generate the shrinkwrap.json, an existing package-lock.json must be present
To generate the `npm-shrinkwrap.json`, an existing `package-lock.json` must be present
and the command [`npm
shrinkwrap`](https://docs.npmjs.com/cli/v8/commands/npm-shrinkwrap) must be
run.
Expand All @@ -215,10 +240,28 @@ run.
1. To add a dependency to a manifest file, ***locally*** run [`npm
install --save <dep-name>`](https://docs.npmjs.com/cli/v8/commands/npm-install).

1. In automated environments (e.g., in CI or production) or when building
artifacts for end-users (e.g., container images, etc):
1. If a project is a library, an `npm-shrinkwrap.json` should ***not*** be published.
The reasoning is that version resolution should be left to the package consumer.
Allow all versions from the minimum to the latest you support, e.g.,
`^m.n.o` to pin to a major range; `~m.n.o` to pin to a minor range. Avoid versions
with critical vulnerabilities as much as possible. Visit the [semver
calculator](https://semver.npmjs.com/) to help you define the ranges.

1. Always generate a package-lock.json ***locally*** and commit it to the
1. If a project is a standalone CLI, developers may publish an `npm-shrinkwrap.json`.
Remember that, by declaring an `npm-shrinkwrap.json`, you take responsibility
for updating all the dependencies in time. Your users will not be able
to update them. If you expect your CLI to be used by other projects and defined
in their package.json or lockfile, do **not** use `npm-shrinkwrap.json` because it will
hinder dependency resolution for your consumers.

1. Projects that do no use a `npm-shrinkwrap.json` (libraries, standalone CLIs
or application projects) should declare and commit a `package-lock.json` (or other dev-only lockfile) to their repository.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the best practice, similar to the shrinkwrap discussion, it depends on the type of project. So only applications should define a package-lock.json and commit it to their repos.

If libraries are using lockfiles they can accidentally publish versions which were only tested with outdated dependencies, so end users might break when they do a fresh install. The goal for library authors should be to have a CI system which does a "fresh" install so that a new user that just runs npm i your-lib will atleast at the time of publishing get the same experience you did in your tests.

For comparison, an application should use a lock file because for your users to "get the same experience as when authored" you need even rebuilds of the same source to produce consistent results. So the lockfile means a build from the same commit will get the same experience you intended at the time of publishing. And you as the author typically control the full experience, as there is no "client building" of the app code like there is with a lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right I botched the part on libraries. I was trying to say the following:

  1. A package-lock.json should be used and committed to the repo for all projects unless they use an npm-shrinkwrap (reasoning: benefits developers' machines as per here). Contributors and maintainers should be encouraged to run npm ci and npm install-ci-test locally.
  2. Libraries (or standalone CLIs that end up in other projects) should always ignore the lock file when running tests in CI (e.g. via npm install --no-package-lock) and should be careful to run these tests in a non-privileged environment.
  3. Applications projects should run CI via npm ci and npm install-ci-test, and should also pay attention to using the less-privileged principles for their CI runs

I'm trying to reconcile the comments in #24 (lockfiles to mitigate malicious dependencies on dev machines) and the fact that it should not be used on libraries (or in projects that end up in other projects' manifest / lockfiles).

An alternative to 1 above would be to tell contributors to define a lockfile locally (in combination with .gitignore). However updates to this file would be manual and difficult to keep track of. Having it in the repository benefits from the commit history of the repo.

Looking forward to your feedback / suggestion. Thanks again!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

friendly ping for feedback
@wesleytodd

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where this captures the actual legitimate use of shrinkwrap files, which is when building and shipping standalone CLIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the text above was just a diff from the PR's content. shrinkwrap was already discussed. Can you take a look at the section If a project is a standalone CLI:?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaa yes I see it there now. Sorry, jumped too fast on this :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wesleytodd do you have further feedback? Are you OK with the latest changes?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really sorry for dropping off here, I totally missed the notification.

I think what landed in the "recommendations" section is technically correct, so glad to see it got sorted out. One thing I do feel here is that it is a quite complicated set of recommendations. I sent the link to a team member at work and they basically just said "I will just remove the package-lock". I think this will be a common response because the guide is technically correct at the cost of being very complicated.

Anyway, like you said below:

I think developers will do in practice regardless of what we say (e.g. when troubleshooting/repdoducing a problem). I think the intention is to raise awareness and encourage users to run these non-locked tests in CI/less-privileged env when possible.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't disagree that we could make the recommendation simpler. Do you think we should include more examples, and possibly links to an example repository which contains lock files, a workflows example, etc?

The reasoning is that this lockfile will provide the benefits highlighted in
[Reproducible installation](#reproducible-installation) by default for privileged environments
(project contributors' machines, CI, production or other environments with access to sensitive data,
such as secrets, PII, write/push permission to the repository, etc). To generate the lockfile:

1. Always generate a `package-lock.json` ***locally*** and commit it to the
repository.

1. Never run commands that may update the lock files or fetch unpinned
Expand All @@ -244,19 +287,24 @@ run.
1. To run tests, run [`npm
install-ci-test`](https://docs.npmjs.com/cli/v8/commands/npm-install-ci-test).

1. If you need to run a CLI package from the registry, ensure the package is a part of
the dependencies defined in your project via the `package.json` file, prior to being installed at build-time in your CI or otherwise automated environment.

1. If a project is a CLI application (`main` entry in the manifest file),
developers may publish a shrinkwrap.json.

1. If a project is a library (`lib` entry in the manifest file), a
shrinkwrap.json should ***not*** be published. The reasoning is that version
resolution should be left to the package consumer. Allow all versions from
the minimum to the latest you support, e.g., `^m.n.o` to pin to a major
range; `~m.n.o` to pin to a minor range. Avoid versions with critical
vulnerabilities as much as possible. Visit the [semver
calculator](https://semver.npmjs.com/) to help you define the ranges.
1. If you need to run a standalone CLI package from the registry, ensure the package is a part of
the dependencies defined in your project via the `package.json` file, prior to
being installed at build-time in your CI or otherwise automated environment.

1. In **non-privileged environments**, maintainers may **ignore** the lockfile. This is particularly useful in
situations where they want to exercise a wide range of dependency versions in order to discover / fix problems before
their users do. This is useful for maintainers of libraries and standalone CLI projects
without an `npm-shrinkwrap.json`. The reasoning is that many downstream users will use `npm install` to install a dependency,
so using floating versions in (non-privileged) tests can be beneficial.

1. If you run CI via GitHub Actions, a non-privileged environment is a workflow **without** access to GitHub secrets and with
non-write permissions defined, such as `permissions: read-all`, `permissions:`, `contents: none`, `contents: read`.
For more information about permissions, refer to the [official documentation](https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token).
You may install the [OpenSSF Scorecard Action](https://github.com/ossf/scorecard-action)
to flag non-read permissions on your project.

1. In a **non-privileged environment**, you may ignore the lockfile by running `npm install --no-package-lock`.
If you are not certain whether the environment you are running is privileged or not, reach out to your security team.

### Maintenance

Expand Down