[BUG] Dependency expressed as git+https in package.json becomes git+ssh in package-lock

[fatmatto]

Current Behavior:

I have two private repository on Gitlab: A and B. A is a dependency of B, B uses npm to install A with the syntax "my-package-a":"git+https://..." In my dev machine, when I run npm install, no error occurs, but the generated package json has several entries for my-package-a, some of those are git+https while others are git+ssh, Adding git+ssh entries breaks CI because I use deploy tokens (basic auth) to let NPM use the private git repositories.

(extract of CI output)

$ npm ci
npm ERR! Error while executing:
npm ERR! /usr/bin/git ls-remote -h -t ssh://git@gitlab.com/xxx/xxx.git
npm ERR! 
npm ERR! error: cannot run ssh: No such file or directory
npm ERR! fatal: unable to fork
npm ERR! 
npm ERR! exited with error code: 128
npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2020-12-20T11_28_27_526Z-debug.log
Cleaning up file based variables
00:01
ERROR: Job failed: exit code 1

This never happened before today (i switched to another computer, so my npm versions must differ)

Updating from npm 6.x.x to 7.x.x didn't solve

npm dedupe solved the problem, but i'm not sure that this is the expected behaviour. If this is expected, I apologize, I could not get it from the docs.

Expected Behavior:

If a npm dependency is added as "git+https" to package.json, then package-lock should NOT turn it into "git+ssh".

Steps To Reproduce:

1. create two projects as private repositories in GitLab (did not try with other git hosted services)
2. Add the first project as dependency of the other project as git+https
3. run npm install in a constrained env such as the CI or a container where you don't have SSH credentials to that git server.

Environment:

• OS: OSX 10.15.6
• Node: v15.4.0
• npm: 6.14.10

[drewpc]

I'm having the same issue after upgrading to npm 7.3.0. I'm afraid I have to roll back to npm 6.x now.

Apparently this is by design? The consistent-resolve.js file in Arborist has the following code:

    return isPath && !relPaths ? `file:${fetchSpec}`
      : isPath ? 'file:' + (toPath ? relpath(toPath, fetchSpec) : fetchSpec)
      : hosted ? 'git+' + hosted.sshurl({ noCommittish: false })
      : type === 'git' ? saveSpec
      // always return something.  'foo' is interpreted as 'foo@' otherwise.
      : rawSpec === '' && raw.slice(-1) !== '@' ? raw
      // just strip off the name, but otherwise return as-is
      : rawSpec

And the test cases for that function all pass, specifically showing that git+https URLs are converted to git+ssh:

t.test('consistent hosted git info urls', t => {
  const expect = 'git+ssh://git@github.com/a/b.git'
  t.equal(cr('a/b'), expect)
  t.equal(cr('github:a/b'), expect)
  t.equal(cr('git+https://github.com/a/b'), expect)
  t.equal(cr('git://github.com/a/b'), expect)
  t.equal(cr('git+ssh://git@github.com/a/b'), expect)
  t.equal(cr('git+https://github.com/a/b.git'), expect)
  t.equal(cr('git://github.com/a/b.git'), expect)
  t.equal(cr('git+ssh://git@github.com/a/b.git'), expect)

  const hash = '#0000000000000000000000000000000000000000'
  t.equal(cr('a/b' + hash), expect + hash)
  t.equal(cr('github:a/b' + hash), expect + hash)
  t.equal(cr('git+https://github.com/a/b' + hash), expect + hash)
  t.equal(cr('git://github.com/a/b' + hash), expect + hash)
  t.equal(cr('git+ssh://git@github.com/a/b' + hash), expect + hash)
  t.equal(cr('git+https://github.com/a/b.git' + hash), expect + hash)
  t.equal(cr('git://github.com/a/b.git' + hash), expect + hash)
  t.equal(cr('git+ssh://git@github.com/a/b.git' + hash), expect + hash)
  t.equal(cr('xyz@a/b' + hash), expect + hash)
  t.equal(cr('xyz@github:a/b' + hash), expect + hash)
  t.equal(cr('xyz@git+https://github.com/a/b' + hash), expect + hash)
  t.equal(cr('xyz@git://github.com/a/b' + hash), expect + hash)
  t.equal(cr('xyz@git+ssh://git@github.com/a/b' + hash), expect + hash)
  t.equal(cr('xyz@git+https://github.com/a/b.git' + hash), expect + hash)
  t.equal(cr('xyz@git://github.com/a/b.git' + hash), expect + hash)
  t.equal(cr('xyz@git+ssh://git@github.com/a/b.git' + hash), expect + hash)
  t.end()
})

[isaacs]

Hmmm... this is tricky.

I can certainly see the need to keep a git+https url as-is if it has auth specified, but the benefit of consistently saving the ssh url is that we can deduplicate the tree more efficiently when you have for example one package depending on git+https://github.com/foo/bar, another depending on git+ssh://git@github.com:foo/bar, and still another on github:foo/bar.

Also, for known git hosts where we have the git sha already (ie, almost all cases where it's saved in package-lock), we always try to fetch the tarball from the CDN first, and fall back to ssh, so I suspect updating it here will hardly matter.

I think a deeper fix is required, because updating the consistentResolve() method on its own won't address the underlying issue.

[drewpc]

Great points; thanks for taking the time to respond! After digging around in the npm 6.x and 7.x codebases, it looks like this issue has come and gone a few times:

-2.x: https://github.com/npm/cli/blob/e09bdc142a4d9fe247927eb82a49720d5370b293/changelogs/CHANGELOG-2.md#lets-git-inky -5.x: https://github.com/npm/cli/blob/e09bdc142a4d9fe247927eb82a49720d5370b293/changelogs/CHANGELOG-5.md#shronkwraps-and-lackfiles -6.x: https://github.com/npm/cli/blob/e09bdc142a4d9fe247927eb82a49720d5370b293/changelogs/CHANGELOG-6.md#shronkwraps-and-lackfiles

As a potential solution, what if the resolved attribute was the actual link (to prevent authentication issues when actually fetching things) and deduplication equivalency tests matched on a common/normalized URL form (e.g. the git+ssh URI)?

It looks like NPM 6.x compares packages here: https://github.com/npm/cli/blob/5caaff41f6748c7c2cbd8c34c3f4a399687806a5/lib/install/diff-trees.js#L93-L98

And NPM 7.x (Arborist) compares packages here: https://github.com/npm/arborist/blob/c31ea7aa7161ca67807a3fd9ca1f5be78e5daa0c/lib/node.js#L902-L915

Both of those modules use hosted-git-info to parse the URIs into objects. What if we added a comparison function to the GitHost object and, instead of matching on the resolved value, we compared the two GitHost objects?

A naive version of this function could be:

GitHost.prototype.matches = function (other) {
  const opts = { noGitPlus: false, noCommittish: false }
  return this.sshurl(opts) === other.this.sshurl(opts)
}

[isaacs]

Fixed by https://github.com/npm/arborist/commit/9989f4f

[CarlosHdez]

Just to confirm, this was fixed for the newest version of npm 7.x.x, right? I'm using 6.x.x and I'm seeing this issue too

[isaacs]

@CarlosHdez Yes, nothing in npm 6.x is changing any more, except high-severity security type issues.

[weskerfoot]

I'm still seemingly getting this issue on node 14.16.0 (???)

[jcoll-busup]

I'm still seemingly getting this issue on node 14.16.0 (???)

Node 14 comes with NPM 6, which as stated by @isaacs, won't be getting this fix.