From d59b3b517a7362edb01bd3cd3c75227f6c462842 Mon Sep 17 00:00:00 2001
From: Philip Harrison <philip@mailharrison.com>
Date: Mon, 23 May 2022 15:02:19 +0100
Subject: [PATCH] Error when passing signature without keys

Raise an error when the package has a signature, `verifySignatures` is true and there are no registry keys configured.

Updated the error message and fixed the undefined name@version in the error message to match the test cases here:
https://github.com/npm/cli/pull/4827

Possible anti-pattern: I've attached a bunch of error attributes if the signature is invalid so we can present this in the json output to ease debugging.
---
 lib/registry.js  | 76 ++++++++++++++++++++++++++----------------------
 test/registry.js | 17 +++++++++--
 2 files changed, 55 insertions(+), 38 deletions(-)

diff --git a/lib/registry.js b/lib/registry.js
index 2eb37bce..16537c86 100644
--- a/lib/registry.js
+++ b/lib/registry.js
@@ -165,43 +165,49 @@ class RegistryFetcher extends Fetcher {
       mani._integrity = String(this.integrity)
       if (dist.signatures) {
         if (this.opts.verifySignatures) {
-          if (this.registryKeys) {
-            // validate and throw on error, then set _signatures
-            const message = `${mani._id}:${mani._integrity}`
-            for (const signature of dist.signatures) {
-              const publicKey = this.registryKeys.filter(key => (key.keyid === signature.keyid))[0]
-              if (!publicKey) {
-                throw Object.assign(new Error(
-                  `${mani._id} has a signature with keyid: ${signature.keyid} ` +
-                  'but no corresponding public key can be found.'
-                ), { code: 'EMISSINGSIGNATUREKEY' })
-              }
-              const validPublicKey =
-                !publicKey.expires || (Date.parse(publicKey.expires) > Date.now())
-              if (!validPublicKey) {
-                throw Object.assign(new Error(
-                  `${mani._id} has a signature with keyid: ${signature.keyid} ` +
-                  `but the corresponding public key has expired ${publicKey.expires}`
-                ), { code: 'EEXPIREDSIGNATUREKEY' })
-              }
-              const verifier = crypto.createVerify('SHA256')
-              verifier.write(message)
-              verifier.end()
-              const valid = verifier.verify(
-                publicKey.pemkey,
-                signature.sig,
-                'base64'
-              )
-              if (!valid) {
-                throw Object.assign(new Error(
-                  'Integrity checksum signature failed: ' +
-                  `key ${publicKey.keyid} signature ${signature.sig}`
-                ), { code: 'EINTEGRITYSIGNATURE' })
-              }
+          // validate and throw on error, then set _signatures
+          const _id = `${mani.name}@${mani.version}`
+          const message = `${_id}:${mani._integrity}`
+          for (const signature of dist.signatures) {
+            const publicKey = this.registryKeys &&
+              this.registryKeys.filter(key => (key.keyid === signature.keyid))[0]
+            if (!publicKey) {
+              throw Object.assign(new Error(
+                `${_id} has a registry signature with keyid: ${signature.keyid} ` +
+                `but no corresponding public key can be found on ${this.registry}-/npm/v1/keys`
+              ), { code: 'EMISSINGSIGNATUREKEY' })
+            }
+            const validPublicKey =
+              !publicKey.expires || (Date.parse(publicKey.expires) > Date.now())
+            if (!validPublicKey) {
+              throw Object.assign(new Error(
+                `${_id} has a registry signature with keyid: ${signature.keyid} ` +
+                `but the corresponding public key on ${this.registry}-/npm/v1/keys ` +
+                `has expired ${publicKey.expires}`
+              ), { code: 'EEXPIREDSIGNATUREKEY' })
+            }
+            const verifier = crypto.createVerify('SHA256')
+            verifier.write(message)
+            verifier.end()
+            const valid = verifier.verify(
+              publicKey.pemkey,
+              signature.sig,
+              'base64'
+            )
+            if (!valid) {
+              throw Object.assign(new Error(
+                `${_id} has an invalid registry signature with ` +
+                `keyid: ${publicKey.keyid} and signature: ${signature.sig}`
+              ), {
+                code: 'EINTEGRITYSIGNATURE',
+                keyid: publicKey.keyid,
+                signature: signature.sig,
+                resolved: mani._resolved,
+                integrity: mani._integrity,
+              })
             }
-            mani._signatures = dist.signatures
           }
-          // if no keys, don't set _signatures
+          mani._signatures = dist.signatures
         } else {
           mani._signatures = dist.signatures
         }
diff --git a/test/registry.js b/test/registry.js
index 046514ea..55dc773b 100644
--- a/test/registry.js
+++ b/test/registry.js
@@ -240,9 +240,15 @@ t.test('verifySignatures invalid signature', async t => {
     }],
   })
   return t.rejects(
+    /abbrev@1\.1\.1 has an invalid registry signature/,
     f.manifest(),
     {
       code: 'EINTEGRITYSIGNATURE',
+      keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA',
+      signature: 'nope',
+      resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz',
+      // eslint-disable-next-line max-len
+      integrity: 'sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==',
     }
   )
 })
@@ -258,6 +264,7 @@ t.test('verifySignatures no valid key', async t => {
   })
   return t.rejects(
     f.manifest(),
+    /@isaacs\/namespace-test@1\.0\.0 has a registry signature/,
     {
       code: 'EMISSINGSIGNATUREKEY',
     }
@@ -271,9 +278,13 @@ t.test('verifySignatures no registry keys at all', async t => {
     verifySignatures: true,
     [`//localhost:${port}/:_keys`]: null,
   })
-  return f.manifest().then(mani => {
-    t.notOk(mani._signatures)
-  })
+  return t.rejects(
+    f.manifest(),
+    /@isaacs\/namespace-test@1\.0\.0 has a registry signature/,
+    {
+      code: 'EMISSINGSIGNATUREKEY',
+    }
+  )
 })
 
 t.test('404 fails with E404', t => {