From e8c274c85991b74ca0bb7cc5c68be647cf5cbbfd Mon Sep 17 00:00:00 2001
From: isaacs <i@izs.me>
Date: Wed, 27 Nov 2019 15:35:42 -0800
Subject: [PATCH] registry: verify integrity when loading manifest

I ran into a weird issue where an options object was being reused
improperly between pacote.manifest() calls, leading to the tree builder
believing that it was safe to replace basically any node in the tree
with any new node it fetched.

While this is clearly a bug downstream, it would've been easier to catch
if pacote could flag this as a problem.

As of this change, if the provided integrity does not share any
algorithms in common with the integrity in the dist field (either
dist.integrity or dist.shasum) then it's concatenated onto the provided
integrity.  If it DOES share any algorithms in common, but the hashes
don't match, then an EINTEGRITY error is raised.

This also lets us more easily upgrade to newer integrity algos in the
future, and clients will transparently update their metadata when this
happens.
---
 lib/registry.js  | 31 ++++++++++++++++++++++++++-----
 test/registry.js | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/lib/registry.js b/lib/registry.js
index 45646d43..530500c6 100644
--- a/lib/registry.js
+++ b/lib/registry.js
@@ -107,11 +107,32 @@ class RegistryFetcher extends Fetcher {
         if (dist) {
           this.resolved = mani._resolved = dist.tarball
           mani._from = this.from
-          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)
+          const distIntegrity = dist.integrity ? ssri.parse(dist.integrity)
+            : dist.shasum ? ssri.fromHex(dist.shasum, 'sha1', this.opts)
+            : null
+          if (distIntegrity) {
+            if (!this.integrity)
+              this.integrity = distIntegrity
+            else if (!this.integrity.match(distIntegrity)) {
+              // only bork if they have algos in common.
+              // otherwise we end up breaking if we have saved a sha512
+              // previously for the tarball, but the manifest only
+              // provides a sha1, which is possible for older publishes.
+              // Otherwise, this is almost certainly a case of holding it
+              // wrong, and will result in weird or insecure behavior
+              // later on when building package tree.
+              for (const algo of Object.keys(this.integrity)) {
+                if (distIntegrity[algo]) {
+                  throw Object.assign(new Error(
+                    `Integrity checksum failed when using ${algo}: `+
+                    `wanted ${this.integrity} but got ${distIntegrity}.`
+                  ), { code: 'EINTEGRITY' })
+                }
+              }
+              // made it this far, the integrity is worthwhile.  accept it.
+              this.integrity = this.integrity.concat(distIntegrity)
+              this.opts.integrity = this.integrity
+            }
           }
         }
         if (this.integrity)
diff --git a/test/registry.js b/test/registry.js
index 8730c6f7..5d63c672 100644
--- a/test/registry.js
+++ b/test/registry.js
@@ -96,6 +96,44 @@ t.test('provide invalid integrity, fails to unpack', async t => {
   })
 })
 
+t.test('provide invalid integrity, fails to manifest', async t => {
+  const f = new RegistryFetcher('@isaacs/namespace-test', {
+    registry,
+    cache,
+    integrity: 'sha512-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=='
+  })
+  return t.rejects(f.manifest(), {
+    code: 'EINTEGRITY',
+  })
+})
+
+t.test('provide different type of integrity, concats', async t => {
+  const f = new RegistryFetcher('@isaacs/namespace-test', {
+    registry,
+    cache,
+    integrity: 'sha_a_le_beef-this_is_a_very_bad_joke_im_so_sorry',
+  })
+  return f.manifest().then(mani =>
+    t.equal(
+      mani._integrity,
+      'sha_a_le_beef-this_is_a_very_bad_joke_im_so_sorry ' +
+      'sha512-5ZYe1LgwHIaag0p9loMwsf5N/wJ4XAuHVNhSO+qulQOXWnyJVuco6IZjo+5u4ZLF/GimdHJcX+QK892ONfOCqQ=='
+    ))
+})
+
+t.test('provide matching integrity, totes ok', async t => {
+  const f = new RegistryFetcher('@isaacs/namespace-test', {
+    registry,
+    cache,
+    integrity: 'sha512-5ZYe1LgwHIaag0p9loMwsf5N/wJ4XAuHVNhSO+qulQOXWnyJVuco6IZjo+5u4ZLF/GimdHJcX+QK892ONfOCqQ==',
+  })
+  return f.manifest().then(mani =>
+    t.equal(
+      mani._integrity,
+      'sha512-5ZYe1LgwHIaag0p9loMwsf5N/wJ4XAuHVNhSO+qulQOXWnyJVuco6IZjo+5u4ZLF/GimdHJcX+QK892ONfOCqQ=='
+    ))
+})
+
 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(() =>