Fix cache integrity check false-positives across multiple registries #7595
Conversation
This prevents broken installs as a result of a package being resolved from a registry that does not return an sha512 integrity field, such as an internal npm registry, where the package was previously cached from a registry that did return a sha512 integrity hash.
|
cc @arcanis |
|
Hey @Blasz! Thanks for working on it! It looks good to me, especially with the tests you've added ⭐
Good question 🤔 Probably not if we always compute both the sha1 and the sha512 regardless of the registry, but I'm not 100% sure there isn't some edge case somewhere in the logic so I prefer to keep it for now.
Yeah, but it would make the code even quite spaghetti 🤔 |
…7595) * Add tests for cache integrity check * Always store sha1 hash as part of cacheIntegrity This prevents broken installs as a result of a package being resolved from a registry that does not return an sha512 integrity field, such as an internal npm registry, where the package was previously cached from a registry that did return a sha512 integrity hash. * Improve cache integrity check error messages
|
This is probably an edge case, but the build server that publishes the Chocolatey package is hitting this error when trying to check out the Yarn Git repo now: It's currently trying to check out the code to |
…arnpkg#7595) * Add tests for cache integrity check * Always store sha1 hash as part of cacheIntegrity This prevents broken installs as a result of a package being resolved from a registry that does not return an sha512 integrity field, such as an internal npm registry, where the package was previously cached from a registry that did return a sha512 integrity hash. * Improve cache integrity check error messages
…arnpkg#7595) * Add tests for cache integrity check * Always store sha1 hash as part of cacheIntegrity This prevents broken installs as a result of a package being resolved from a registry that does not return an sha512 integrity field, such as an internal npm registry, where the package was previously cached from a registry that did return a sha512 integrity hash. * Improve cache integrity check error messages
Summary
Improve cache integrity error messages and fixes the cache integrity check issue described here: #7584 (comment)
I'm still not so sure about erroring out in these scenarios rather than just invalidating the cache for the problem packages since users will manually need to do that after the error was thrown.
Also now that the
cacheIntegrityfield from cache is being used, I wonder whether the separate-integrityand no-integrity suffixed caches are required anymore?Test plan
Added tests for the previous cache integrity changes. I saw test cases for sha384 integrity hashes so I wonder if those need to be accounted for, as the same false positive error will occur there as well. If npm ever decides to change their default integrity hash to something other than sha512 this check would also fail.
A better way of handling this could be to compute the remote integrity hash against the cache if it did not match the hash algorithms already stored in cache and then append that value to the cached integrity hash if it matched and throw an error otherwise.