Improve integrity consistency and handling

This makes the 'this.integrity' a getter setter that updates the
options, so that the integrity being passed to other parts of the system
is kept in sync.

The setter ensures that we don't ever CHANGE the integrity value (which
could indicate at least a mistake, if not a security exploit), but does
allow for updating with other algorithms, and continually verifying that
the integrity expected for an artifact in one place is also matching
what is expected in another.

Since we have a mix of SHA-1 and SHA-512 in our stack, this always
ensures that we're always using the strongest algorithm available for
integrity verification, but falling back to SHA-1 when SHA-512 is not
available (yet).

Using SHA-512 for make-fetch-happen's caching also increases our cache
hit rate, since we default to that most of the time.

lib/fetcher.js

@@ -49,7 +49,15 @@ class FetcherBase {
     this.opts = opts
     this.cache = opts.cache || cacheDir()
     this.resolved = opts.resolved || null
-    this.integrity = opts.integrity || null
+
+    // default to caching/verifying with sha512, that's what we usually have
+    // need to change this default, or start overriding it, when sha512
+    // is no longer strong enough.
+    this.defaultIntegrityAlgorithm = opts.defaultIntegrityAlgorithm || 'sha512'
+
+    if (typeof opts.integrity === 'string')
+      opts.integrity = ssri.parse(opts.integrity)
+
     this.package = null
     this.type = this.constructor.name
     this.fmode = opts.fmode || 0o666
@@ -96,6 +104,24 @@ class FetcherBase {
     ]
   }
 
+  get integrity () {
+    return this.opts.integrity || null
+  }
+  set integrity (i) {
+    if (!i)
+      return
+
+    i = ssri.parse(i)
+    const current = this.opts.integrity
+
+    // do not ever update an existing hash value, but do
+    // merge in NEW algos and hashes that we don't already have.
+    if (current)
+      current.merge(i)
+    else
+      this.opts.integrity = i
+  }
+
   get notImplementedError () {
     return new Error('not implemented in this fetcher type: ' + this.type)
   }
@@ -150,19 +176,23 @@ class FetcherBase {
     return cacache.get.stream.byDigest(this.cache, this.integrity, this.opts)
   }
 
-  // XXX it looks like Remote and Registry might get this functionality
-  // from make-fetch-happen already.  Probably only need it for git, file,
-  // and dir fetchers, or else we're double-dosing SSRI which makes us slow.
-  // We can't rely on the stream being an IntegrityStream, because MFH will
-  // wrap it up in a MinipassPipeline in many cases.
   [_istream] (stream) {
     // everyone will need one of these, either for verifying or calculating
+    // We always set it, because we have might only have a weak legacy hex
+    // sha1 in the packument, and this MAY upgrade it to a stronger algo.
+    // If we had an integrity, and it doesn't match, then this does not
+    // override that error; the istream will raise the error before it
+    // gets to the point of re-setting the integrity.
     const istream = ssri.integrityStream(this.opts)
-    if (!this.integrity)
-      istream.on('integrity', i => this.integrity = i)
+    istream.on('integrity', i => this.integrity = i)
     return stream.on('error', er => istream.emit('error', er)).pipe(istream)
   }
 
+  pickIntegrityAlgorithm () {
+    return this.integrity ? this.integrity.pickAlgorithm(this.opts)
+      : this.defaultIntegrityAlgorithm
+  }
+
   // TODO: check error class, once those are rolled out to our deps
   isDataCorruptionError (er) {
     return er.code === 'EINTEGRITY' || er.code === 'Z_DATA_ERROR'

lib/registry.js

@@ -106,13 +106,15 @@ class RegistryFetcher extends Fetcher {
         const { dist } = mani
         if (dist) {
           this.resolved = mani._resolved = dist.tarball
-          if (dist.integrity)
-            this.integrity = ssri.parse(dist.integrity)
-          else if (dist.shasum)
-            this.integrity = ssri.fromHex(dist.shasum, 'sha1', this.opts)
-          if (this.integrity)
-            mani._integrity = String(this.integrity)
+          if (!this.integrity) {
+            if (dist.integrity)
+              this.integrity = ssri.parse(dist.integrity)
+            else if (dist.shasum)
+              this.integrity = ssri.fromHex(dist.shasum, 'sha1', this.opts)
+          }
         }
+        if (this.integrity)
+          mani._integrity = String(this.integrity)
         return this.package = mani
       })
   }
@@ -121,7 +123,6 @@ class RegistryFetcher extends Fetcher {
     // we use a RemoteFetcher to get the actual tarball stream
     return new RemoteFetcher(this.resolved, {
       ...this.opts,
-      integrity: this.integrity,
       resolved: this.resolved,
       pkgid: `registry:${this.spec.name}@${this.resolved}`,
     })[_tarballFromResolved]()

lib/remote.js

@@ -23,19 +23,13 @@ class RemoteFetcher extends Fetcher {
       headers: this[_headers](),
       spec: this.spec,
       integrity: this.integrity,
-
-      // Using sha1 is faster here. We'll still calculate sha512 of
-      // the response and verify that against the cached data.  This
-      // is just for make-fetch-happen's fast cache lookup.
-      algorithms: [
-        this.integrity ? ssri.parse(this.integrity).pickAlgorithm() : 'sha1'
-      ],
+      algorithms: [ this.pickIntegrityAlgorithm() ],
     }
     fetch(this.resolved, fetchOpts).then(res => {
       const hash = res.headers.get('x-local-cache-hash')
-
-      if (hash && !this.integrity)
-        this.integrity = ssri.parse(decodeURIComponent(hash))
+      if (hash) {
+        this.integrity = decodeURIComponent(hash)
+      }
 
       res.body.on('error',
         /* istanbul ignore next - exceedingly rare and hard to simulate */
@@ -55,6 +49,7 @@ class RemoteFetcher extends Fetcher {
       'pacote-version': pacoteVersion,
       'pacote-req-type': 'tarball',
       'pacote-pkg-id': this.pkgid,
+      'pacote-integrity': String(this.integrity),
     }
   }
 

package.json

@@ -47,7 +47,7 @@
     "promise-retry": "^1.1.1",
     "read-package-json": "^2.1.0",
     "semver": "^6.3.0",
-    "ssri": "^7.0.1",
+    "ssri": "^7.1.0",
     "tar": "^5.0.5",
     "which": "^2.0.1"
   },

test/fetcher.js

@@ -393,3 +393,19 @@ t.test('make bins executable', async t => {
   t.matchSnapshot(res, 'results of unpack')
   t.equal(fs.statSync(target + '/script.js').mode & 0o111, 0o111)
 })
+
+t.test('set integrity, pick default algo', t => {
+  const opts = {
+    integrity: 'sha1-foobar sha256-barbaz sha512-glorp',
+    defaultIntegrityAlgorithm: 'sha256',
+  }
+  const f = new FileFetcher('pkg.tgz', opts)
+  t.equal(f.pickIntegrityAlgorithm(), 'sha512')
+  t.isa(opts.integrity, Object)
+  const i = f.integrity
+  f.integrity = null
+  t.equal(f.integrity, i, 'cannot remove integrity')
+  const g = new FileFetcher('pkg.tgz', { defaultIntegrityAlgorithm: 'sha256' })
+  t.equal(g.pickIntegrityAlgorithm(), 'sha256')
+  t.end()
+})

test/registry.js

@@ -72,7 +72,7 @@ t.test('underscore, no tag or version', t => {
   .then(() => f.extract(me + '/underscore'))
   .then(result => t.deepEqual(result, {
     resolved: `${registry}underscore/-/underscore-1.5.1.tgz`,
-    integrity: 'sha1-0r3oF9F2/63olKtxRY5oKhS4bck=',
+    integrity: 'sha1-0r3oF9F2/63olKtxRY5oKhS4bck= sha512-yOc7VukmA45a1D6clUn1mD7Mbc9LcVYAQEXNKSTblzha59hSFJ6cAt90JDoxh05GQnTPI9nk4wjT/I8C/nAMPw==',
   }))
 })
 
@@ -88,6 +88,17 @@ t.test('scoped, no tag or version', t => {
   }))
 })
 
+t.test('provide invalid integrity, fails to unpack', async t => {
+  const f = new RegistryFetcher('@isaacs/namespace-test', {
+    registry,
+    cache,
+    integrity: 'sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=='
+  })
+  return t.rejects(f.extract(me + '/bad-integrity'), {
+    code: 'EINTEGRITY',
+  })
+})
+
 t.test('404 fails with E404', t => {
   const f = new RegistryFetcher('thing-is-not-here', {registry, cache})
   return t.rejects(f.resolve(), { code: 'E404' }).then(() =>