Prototype a way to review NPM lockfiles in PRs

[timmc-edx]

When someone submits a PR with changes to Javascript dependencies, it generally involves changed to package.json (a set of dependencies and version constraints) as well as package-lock.json (the output of the dependency solver, as a lockfile). package.json is easy to review, but package-lock.json diffs can be huge, and manually reviewing them is generally infeasible.

This ticket is to create a way to review package-lock.json changes to prevent accidental mismatches from being committed. Malicious PRs are not in scope.

Acceptance criteria:

• A GitHub workflow added to some repo that will detect at least the following kinds of errors:
  • Changes made to package.json but npm install has not been run since last change (which would update package-lock.json)
  • Well-intentioned manual editing of package-lock.json
• A ticket created for generalizing this to other repos (make reusable workflow in openedx/.github and use it from other repos)

Notes:

• GitHub has introduced a "rich view" for package files that summarizes their changes, which is helpful but not a complete solution (as adding one entry to package.json can add 60 entries to package-lock.json).
• npm ci will error if there are certain kinds of mismatches between the two files
• npm ls will detect certain kinds of mismatches between package-lock.json and the node_modules directory, after running npm ci

[jmbowman]

I think npm ls --all --package-lock-only would let us detect mismatches without running npm ci first, but we should confirm that.

[Syed-Ali-Abbas-Zaidi]

Changes made to package.json but npm install has not been run since last change (which would update package-lock.json)

Hi @timmc-edx, Regarding the point where package.json is changed but package-lock.json is not updated, most repositories use npm ci, which already fulfills this acceptance criterion.
Here is a working example.

However, if someone manually changes package-lock.json, it can create a mismatch, and npm ci will not be able to detect it. To identify this type of mismatch, we can use npm ls --all --package-lock-only. We should consider adding this step to our lockfile-check shared workflow. What do you think about this suggestion?

[timmc-edx]

Some experimentation:

• Changing the name or version of something in the packages dict of the lockfile
  • ✔️ Caught by npm ci --dry-run
  • ✔️ Caught by npm ls --all --package-lock-only
• Adding a package to package.json
  • ✔️ Caught by npm ci --dry-run
  • ✔️ Caught by npm ls --all --package-lock-only
• Changing the version number of a package in package.json
  • ✔️ Caught by npm ci --dry-run
  • ✔️ Caught by npm ls --all --package-lock-only
• Deleting a package from package.json, or adding it to package-lock.json (e.g. "@commitlint/cli": "17.6.6" in frontend-app-profile)
  • ❌ Missed by npm ci --dry-run
  • ❌ Sort of caught by npm ls --all --package-lock-only but no exit code is generated.

So they're mostly equivalent, but that last one is a bit concerning; extraneous packages in the lockfile is exactly the kind of thing I'd want to catch. We can use the JSON output to get the actual list of problems, though! So the CI check should look something like this:

problems=$(npm ls --all --package-lock-only --json | jq '.problems[]?' -r)
if [[ -n "$problems" ]]; then
  echo "$problems"
  echo
  echo "Mismatch between package.json and package-lock.json. Please regenerate package lock file with 'npm install'."
  exit 1
fi

This still won't catch certain malicious edits (still researching that), but it should cover most well-intentioned mistakes.

[Syed-Ali-Abbas-Zaidi]

problems=$(npm ls --all --package-lock-only --json | jq '.problems[]?' -r)
if [[ -n "$problems" ]]; then
echo "$problems"
echo
echo "Mismatch between package.json and package-lock.json. Please regenerate package lock file with 'npm install'."
exit 1
fi

We are planning to add the suggested step to this lockfile check workflow.