tools: add package-lock when installing npm dependency

[marco-ippolito]

Adding package lock with npm version when installing a dependency with npm.
This allow us to have a better view of what we are installing
Refs: nodejs/security-wg#1037
If we agree on this I'll do for the rest of npm deps

[nodejs-github-bot]

Review requested:

• @nodejs/security-wg

[RafaelGSS]

cc: @nodejs/build @nodejs/tsc for visibility

[targos]

Can you execute the script and commit the package-lock so we see how it looks like?

[marco-ippolito]

8a8551a
It gives some useful infos like npm version, sha etc...

[ruyadorno]

I'm not convinced of the benefits of adding ALL that extra metadata to the repo, given that the deps are already checked in. Are there other less-obvious benefits?

[marco-ippolito]

Have all the infos to make the installation repeatable, check whats in the dependency tree

[aduh95]

Why would we need to make the installation repeatable using npm rather than git?

[marco-ippolito]

Probably the package lock is a bit misleading.
We install this dependency by creating npm init + npm install acorn, and then we copy the node modules into the deps folder. This is the package lock of the installation.
I think its a good idea to keep track of what npm version we have used when installing something and the metadata of dependency.
I dont see any con of having one files more in source, and the pros are knowing what you are actually pulling in (visibility on the supply chain) and how you installed it.
Like I always want the package lock in my source code

[mhdawson]

Why would we need to make the installation repeatable using npm rather than git?

@aduh95 the issue is that if we have to make a change in the future to fix a CVE that may require building from the original source not the transformed version that we use when build Node.js. Doing that will require all of the tools needed for the original build to create the transformed version. Unless we have the exact some versions of those we may introduce unintended changes beyond just the change we are trying to make for the CVE. Related if there is a CVE in one of those deps, unless we know the exact version used we will not be able to tell if Node.js is affected or not.

[mhdawson]

LGTM

[mhdawson]

Adding to TSC agenda so I can address any remaining questions/comments since we had questions from a few TSC members.

[marco-ippolito]

so the package-lock doesn't match with the package because of the way the package is generated:

• npm init
• npm install acorn-walk
• move /node_modules/acor-walk to dep/acornwalk

the right way would be download the directly acorn-walk (as tar, zip) without going through npm installation, and therefore without needing a package-lock, so it might be required to change the update script

[richardlau]

8a8551a It gives some useful infos like npm version, sha etc...

For reference, the package-lock.json from that commit looks like this:

{
  "name": "acorn-walk-tmp",
  "version": "1.0.0",
  "lockfileVersion": 3,
  "requires": true,
  "packages": {
    "": {
      "name": "acorn-walk-tmp",
      "version": "1.0.0",
      "license": "ISC",
      "dependencies": {
        "acorn-walk": "^8.2.0"
      },
      "engines": {
        "npm": "10.1.0"
      }
    },
    "node_modules/acorn-walk": {
      "version": "8.2.0",
      "resolved": "https://registry.npmjs.org/acorn-walk/-/acorn-walk-8.2.0.tgz",
      "integrity": "sha512-k+iyHEuPgSw6SbuDpGQM+06HQUa04DZ3o+F6CSzXMvvI5KMvnaEqXe+YVe555R9nn6GPt404fos4wcgpw12SDA==",
      "engines": {
        "node": ">=0.4.0"
      }
    }
  }
}

The point I was raising in yesterday's TSC meeting was that this looks very odd -- acorn-walk-tmp is a completely artificial, out-of-tree construct. It's not obvious how this relates to the source tree in this repository unless you are familiar with how the updates are done.

[marco-ippolito]

I will close this PR and create another one to change the update script