Add check to ensure we actually get our commit

[klassiker]

In npm/rfcs#525 about ignoring integrity values in lockfiles it was stated:

the sha is already what gets stored in the resolved field today

This is only true for resolutions from non-commits to commits.

A dependency like git://...#4b559c4c663a23f988f6be5094c9a45faf6231bc will be stored using the same "reference" in resolved even when it cloned a branch or a tag that resolved to a different sha.

The update is only done if it hasn't been resolved yet, which is already the case if a full "commit" was specified:

pacote/lib/git.js

Lines 263 to 265 in 4b559c4

	if (!this.resolved) {
	await this.#addGitSha(sha)
	}

This also applies to npm ci after reading package-lock.json as it will use the same resolution.

This will compare the newly returned commit-hash with a previously set resolvedSha and prevent that from happening.

[wraithgar]

This is introducing a new integrity check to pacote itself. Typically pacote doesn't actually do the integrity check. Its responsibility is to assign the integrity (if one exists) and let the consumer decide if it needs to be checked. You can see in the commit that removed this for git refs how we are bypassing it.

You can also see where we are ignoring integrity altogether when we clone git repos.

If I understand correctly what is happening here is nothing more than checking that if we have a resolvedSha (which we set to the value of the commit sha in the git reference), we fetched that sha. But ... that's how the code is already set up. We have to trust that if we ask for a ref that's what our code does.

What is this protecting against? Editing the package.json without updating the lockfile? I think understanding HOW these could be different will help inform how we want to approach this, and from where we want to throw the error.

Finally, when and if we do throw we need it to effectively be the same kind of error as an integrity error. So a code will need to be attached that is either EINTEGRITY or something new.

[klassiker]

If I understand correctly what is happening here is nothing more than checking that if we have a resolvedSha (which we set to the value of the commit sha in the git reference), we fetched that sha. But ... that's how the code is already set up. We have to trust that if we ask for a ref that's what our code does.

Cloning in @npmcli/git chooses a strategy based on revDoc, which was returned by resolveRef based on the output of a previous call to git ls-remote:
https://github.com/npm/git/blob/6dc0aefb44b66c9f2f941c5182dabbe549bb703b/lib/clone.js#L52-L63

For example, these conditions will replace an existing resolvedSha:

• the HEAD moves between calls to git ls-remote and git clone for plain
• a branch or tag with the same name will be cloned in branch
• same if one gets created right before git clone --mirror with unresolved

For me, there are two main cases where we already have a resolved sha:

1. git dependency with full commit-ish pin in the package.json
npm install writes the incorrect commit to package-lock.json as resolved is already set
2. resolved commit inside a package-lock.json
npm ci wouldn't even detect changes to resolved and clones a different commit

What is this protecting against? Editing the package.json without updating the lockfile? I think understanding HOW these could be different will help inform how we want to approach this, and from where we want to throw the error.

We should protect the consumer of the cloned temporary directory from using a different commit than requested. This change only trusts @npmcli/git to report the correct sha that was cloned, similar to how it is used for resolving. This is easier to ensure without changing much (see npm/git#232).

[wraithgar]

Ok thanks for the detailed response. The only thing left then is to make sure we are attaching an appropriate code to the thrown error to help npm (and other consumers) inform the user better.

[klassiker]

The only thing left then is to make sure we are attaching an appropriate code to the thrown error

I've enabled edits by maintainers, not sure what you need here

[wraithgar]

Something like this: https://github.com/npm/cacache/blob/d171fe9c9f5ccdcd27a9c68e965ca6056de76f87/lib/content/read.js#L161

EINTEGRITY is pretty coupled to tarball integrity and ssri so it's probably not going to be that exact code. It should be something so that npm can pull it out and make a more helpful error message. It may help to attach the sha we were expecting to the error object too as sha.