Use pacote.manifest and cleanup output

Author: feelepxyz

lib/commands/audit.js

@@ -6,12 +6,14 @@ const fetch = require('npm-registry-fetch')
 const localeCompare = require('@isaacs/string-locale-compare')('en')
 const npa = require('npm-package-arg')
 const pacote = require('pacote')
-const pickManifest = require('npm-pick-manifest')
-const table = require('text-table')
+// const pickManifest = require('npm-pick-manifest')
 
 const ArboristWorkspaceCmd = require('../arborist-cmd.js')
-const ansiTrim = require('../utils/ansi-trim.js')
 const auditError = require('../utils/audit-error.js')
+const {
+  registry: { default: defaultRegistry },
+} = require('../utils/config/definitions.js')
+// const log = require('../utils/log-shim.js')
 const reifyFinish = require('../utils/reify-finish.js')
 
 const validateSignature = async ({ message, signature, publicKey }) => {
@@ -37,27 +39,26 @@ class VerifySignatures {
   }
 
   async run () {
+    const start = process.hrtime.bigint()
+
     // Find all deps in tree
     const nodes = this.tree.inventory.values()
     this.getEdges(nodes, 'edgesOut')
-
     const edges = Array.from(this.edges)
-    const start = process.hrtime.bigint()
-    const registries = this.findAllRegistryUrls(edges, this.npm.flatOptions)
 
-    // Prefetch and cache public keys from the used registries
+    // QUESTION: Do we need to get the registry host from the resolved url to handle proxies?
+    // Prefetch and cache public keys from used registries
+    const registries = this.findAllRegistryUrls(edges, this.npm.flatOptions)
     for (const registry of registries) {
       const keys = await this.getKeys({ registry })
       if (keys) {
         this.keys.set(registry, keys)
       }
     }
-    await Promise.all(edges.map((edge) => this.getVerifiedInfo(edge)))
 
-    const end = process.hrtime.bigint()
-    const elapsed = end - start
+    await Promise.all(edges.map((edge) => this.getVerifiedInfo(edge)))
 
-    // sort alphabetically
+    // Sort alphabetically
     const invalid = Array.from(this.invalid).sort((a, b) => localeCompare(a.name, b.name))
     const missing = Array.from(this.missing).sort((a, b) => localeCompare(a.name, b.name))
 
@@ -67,13 +68,20 @@ class VerifySignatures {
       this.exitCode = 1
     }
 
+    const end = process.hrtime.bigint()
+    const elapsed = end - start
+
     if (this.npm.config.get('json')) {
       this.appendOutput(this.makeJSON({ invalid, missing }))
     } else {
       const timing = `audited ${edges.length} packages in ${Math.floor(Number(elapsed) / 1e9)}s`
       const verifiedPrefix = verified ? 'verified signatures, ' : ''
       this.appendOutput(`${verifiedPrefix}${timing}\n`)
 
+      if (this.verified && !verified) {
+        this.appendOutput(`${this.verified} ${chalk.bold('verified')} packages\n`)
+      }
+
       if (missing.length) {
         const msg = missing.length === 1 ?
           `package has a ${chalk.bold(chalk.magenta('missing'))} registry signature` :
@@ -183,6 +191,7 @@ class VerifySignatures {
     }
   }
 
+  // TODO: Remove this once we can get time from pacote.manifest
   async getPackument (spec) {
     const packument = await pacote.packument(spec, {
       ...this.npm.flatOptions,
@@ -218,6 +227,7 @@ class VerifySignatures {
     const node = edge.to || edge
     const { path, location } = node
     const { version } = node.package || {}
+    // QUESTION: Do we need to handle `latest`?
     if (!version) {
       return
     }
@@ -232,51 +242,56 @@ class VerifySignatures {
       }
     }
 
+    // QUESTION: Confirm, is this the right thing to do here?
+    //
     // deps different from prod not currently
     // on disk are not included in the output
     if (edge.error === 'MISSING' && type !== 'dependencies') {
       return
     }
 
+    // QUESTION: Confirm, is this the right thing to do here?
+    //
+    // if it's not a range, version, or tag, skip it
     try {
-      const packument = await this.getPackument(spec)
-      // if it's not a range, version, or tag, skip it
-      try {
-        if (!npa(`${edge.name}@${edge.spec}`).registry) {
-          return null
-        }
-      } catch (err) {
+      if (!npa(`${edge.name}@${edge.spec}`).registry) {
         return null
       }
+    } catch (err) {
+      return null
+    }
 
+    try {
       const name = alias ? edge.spec.replace('npm', edge.name) : edge.name
-      const { homepage } = packument
-
+      // QUESTION: Is name@version the right way to get the manifest?
+      const manifest = await pacote.manifest(`${name}@${version}`, this.npm.flatOptions)
       const registry = fetch.pickRegistry(spec, this.npm.flatOptions)
 
-      const versionPackument = pickManifest(packument, version, this.npm.flatOptions)
-      const versionCreated = packument.time && packument.time[version]
-      const dist = versionPackument.dist || {}
-      const { integrity } = dist
+      const { _integrity: integrity, _signatures } = manifest
       const message = `${name}@${version}:${integrity}`
-      const signatures = dist.signatures || []
+      const signatures = _signatures || []
+
+      // TODO: Get version created time from manifest
+      //
+      // const packument = await this.getPackument(spec)
+      // const versionCreated = packument.time && packument.time[version]
       const keys = this.keys.get(registry) || []
       const validKeys = keys.filter((publicKey) => {
         if (!publicKey.expires) {
           return true
         }
-        return Date.parse(versionCreated) < Date.parse(publicKey.expires)
+        // return Date.parse(versionCreated) < Date.parse(publicKey.expires)
+        return Date.parse(publicKey.expires) > Date.now()
       })
 
       // Currently we only care about missing signatures on registries that provide a public key
-      // Note: we could make this configurable in the future with a strict/paranoid mode
+      // We could make this configurable in the future with a strict/paranoid mode
       if (!signatures.length && validKeys.length) {
         this.missing.add({
           name,
           path,
           version,
           location,
-          homepage,
           registry,
         })
 
@@ -313,7 +328,6 @@ class VerifySignatures {
             type,
             version,
             location,
-            homepage,
             registry,
             integrity,
             signature: signature.sig,
@@ -324,6 +338,8 @@ class VerifySignatures {
         }
       }))
     } catch (err) {
+      // QUESTION: Is this the right way to handle these errors?
+      //
       // silently catch and ignore ETARGET, E403 &
       // E404 errors, deps are just skipped
       if (!(
@@ -337,52 +353,18 @@ class VerifySignatures {
   }
 
   humanOutput (list) {
-    const invalidList = list.map(x => this.makePretty(x))
-    const outHead = ['Package',
-      'Version',
-      'Location',
-      'Registry',
-    ]
-
-    if (this.npm.config.get('long')) {
-      outHead[4] = 'Homepage'
-    }
-
-    const outTable = [outHead].concat(invalidList)
-
-    if (this.npm.color) {
-      outTable[0] = outTable[0].map(heading => chalk.underline(heading))
-    }
-
-    const tableOpts = {
-      align: ['l', 'r', 'r', 'r', 'l'],
-      stringLength: s => ansiTrim(s).length,
-    }
-
-    return table(outTable, tableOpts)
-  }
-
-  // formatting functions
-  makePretty (dep) {
-    const {
-      version = 'MISSING',
-      homepage = '',
-      name,
-      location,
-      registry,
-    } = dep
-
-    const columns = [name, version, location, registry]
-
-    if (this.npm.config.get('long')) {
-      columns[4] = homepage
-    }
-
-    if (this.npm.color) {
-      columns[0] = chalk.red(columns[0])
-    }
+    const uniquePackages = Array.from(list.reduce((set, v) => {
+      let nameVersion = `${v.name}@${v.version}`
+      if (this.npm.color) {
+        nameVersion = chalk.red(nameVersion)
+      }
+      const registry = v.registry
+      const suffix = registry !== defaultRegistry ? ` (${registry})` : ''
+      set.add(`${nameVersion}${suffix}`)
+      return set
+    }, new Set()))
 
-    return columns
+    return uniquePackages.join('\n')
   }
 
   makeJSON ({ invalid, missing }) {
@@ -396,7 +378,6 @@ class VerifySignatures {
         integrity,
         signature,
         keyid,
-        homepage,
       } = dep
       out.invalid = out.invalid || {}
       out.invalid[name] = {
@@ -406,7 +387,6 @@ class VerifySignatures {
         signature,
         integrity,
         keyid,
-        homepage,
       }
     })
     missing.forEach(dep => {
@@ -415,14 +395,12 @@ class VerifySignatures {
         version,
         path,
         registry,
-        homepage,
       } = dep
       out.missing = out.invalid || {}
       out.missing[name] = {
         version,
         location: path,
         registry,
-        homepage,
       }
     })
     return JSON.stringify(out, null, 2)