From 0def9845e95a9a9274878c8be55f3e3d6b976d65 Mon Sep 17 00:00:00 2001 From: Philip Harrison Date: Thu, 12 May 2022 16:09:23 +0100 Subject: [PATCH] Add a working test --- lib/commands/audit.js | 166 +++++++--------- .../test/lib/commands/audit.js.test.cjs | 10 +- test/lib/commands/audit.js | 183 +++++++++++------- 3 files changed, 185 insertions(+), 174 deletions(-) diff --git a/lib/commands/audit.js b/lib/commands/audit.js index a2453f02c4f48..d0013906efbeb 100644 --- a/lib/commands/audit.js +++ b/lib/commands/audit.js @@ -6,14 +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 ArboristWorkspaceCmd = require('../arborist-cmd.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 log = require('../utils/log-shim.js') +const pulseTillDone = require('../utils/pulse-till-done.js') const reifyFinish = require('../utils/reify-finish.js') const validateSignature = async ({ message, signature, publicKey }) => { @@ -45,8 +45,10 @@ class VerifySignatures { const nodes = this.tree.inventory.values() this.getEdges(nodes, 'edgesOut') const edges = Array.from(this.edges) + if (edges.length === 0) { + throw new Error('No dependencies found') + } - // 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) { @@ -75,11 +77,13 @@ class VerifySignatures { 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, ' : '' + const verifiedPrefix = verified ? 'verified registry signatures, ' : '' this.appendOutput(`${verifiedPrefix}${timing}\n`) if (this.verified && !verified) { - this.appendOutput(`${this.verified} ${chalk.bold('verified')} packages\n`) + this.appendOutput( + `${this.verified} packages have ${chalk.bold('verified')} registry signatures\n` + ) } if (missing.length) { @@ -191,16 +195,6 @@ 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, - fullMetadata: this.npm.config.get('long'), - preferOffline: true, - }) - return packument - } - async getKeys ({ registry }) { return await fetch.json('/-/npm/v1/keys', { ...this.npm.flatOptions, @@ -260,95 +254,75 @@ class VerifySignatures { return null } - try { - const name = alias ? edge.spec.replace('npm', edge.name) : edge.name - // 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 { _integrity: integrity, _signatures } = manifest - const message = `${name}@${version}:${integrity}` - 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(publicKey.expires) > Date.now() + const name = alias ? edge.spec.replace('npm', edge.name) : edge.name + const manifest = await pacote.manifest(`${name}@${version}`, this.npm.flatOptions) + const registry = fetch.pickRegistry(spec, this.npm.flatOptions) + + const { _integrity: integrity, _signatures } = manifest + const message = `${name}@${version}:${integrity}` + const signatures = _signatures || [] + + const keys = this.keys.get(registry) || [] + const validKeys = keys.filter((publicKey) => { + if (!publicKey.expires) { + return true + } + return Date.parse(publicKey.expires) > Date.now() + }) + + // Currently we only care about missing signatures on registries that provide a public key + // 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, + registry, + }) + + return + } + + await Promise.all(signatures.map(async (signature) => { + const publicKey = keys.filter(key => key.keyid === signature.keyid)[0] + const validPublicKey = validKeys.filter(key => key.keyid === signature.keyid)[0] + + if (!publicKey && !validPublicKey) { + throw new Error( + `${name} has a signature with keyid: ${signature.keyid} ` + + `but not corresponding public key can be found on ${registry}-/npm/v1/keys` + ) + } else if (publicKey && !validPublicKey) { + throw new Error( + `${name} has a signature with keyid: ${signature.keyid} ` + + `but the corresponding public key on ${registry}-/npm/v1/keys has expired ` + + `(${publicKey.expires})` + ) + } + + const valid = await validateSignature({ + message, + signature: signature.sig, + publicKey: validPublicKey.pemKey, }) - // Currently we only care about missing signatures on registries that provide a public key - // We could make this configurable in the future with a strict/paranoid mode - if (!signatures.length && validKeys.length) { - this.missing.add({ + if (!valid) { + this.invalid.add({ name, path, + type, version, location, registry, - }) - - return - } - - await Promise.all(signatures.map(async (signature) => { - const publicKey = keys.filter(key => key.keyid === signature.keyid)[0] - const validPublicKey = validKeys.filter(key => key.keyid === signature.keyid)[0] - - if (!publicKey && !validPublicKey) { - throw new Error( - `${name} has a signature with keyid: ${signature.keyid} ` + - `but not corresponding public key can be found on ${registry}-/npm/v1/keys` - ) - } else if (publicKey && !validPublicKey) { - throw new Error( - `${name} has a signature with keyid: ${signature.keyid} ` + - `but the corresponding public key on ${registry}-/npm/v1/keys has expired ` + - `(${publicKey.expires})` - ) - } - - const valid = await validateSignature({ - message, + integrity, signature: signature.sig, - publicKey: validPublicKey.pemKey, + keyid: signature.keyid, }) - - if (!valid) { - this.invalid.add({ - name, - path, - type, - version, - location, - registry, - integrity, - signature: signature.sig, - keyid: signature.keyid, - }) - } else { - this.verified++ - } - })) - } 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 (!( - err.code === 'ETARGET' || - err.code === 'E403' || - err.code === 'E404') - ) { - throw err + } else { + this.verified++ } - } + })) } humanOutput (list) { @@ -471,6 +445,7 @@ class Audit extends ArboristWorkspaceCmd { } async auditSignatures () { + log.newItem('loading intalled packages') const reporter = this.npm.config.get('json') ? 'json' : 'detail' const opts = { ...this.npm.flatOptions, @@ -494,8 +469,9 @@ class Audit extends ArboristWorkspaceCmd { arb.excludeWorkspacesDependencySet(tree) } + log.newItem('verifying registry signatures') const verify = new VerifySignatures(tree, filterSet, this.npm, { ...opts }) - await verify.run() + await pulseTillDone.withPromise(verify.run()) const result = verify.report() process.exitCode = process.exitCode || result.exitCode this.npm.output(result.report) diff --git a/tap-snapshots/test/lib/commands/audit.js.test.cjs b/tap-snapshots/test/lib/commands/audit.js.test.cjs index 8fec03aa30c08..b606c522395d9 100644 --- a/tap-snapshots/test/lib/commands/audit.js.test.cjs +++ b/tap-snapshots/test/lib/commands/audit.js.test.cjs @@ -41,6 +41,11 @@ added 1 package, and audited 2 packages in xxx found 0 vulnerabilities ` +exports[`test/lib/commands/audit.js TAP audit signatures signature verification with valid signatures > must match snapshot 1`] = ` +verified registry signatures, audited 1 packages in xxx + +` + exports[`test/lib/commands/audit.js TAP fallback audit > must match snapshot 1`] = ` # npm audit report @@ -124,8 +129,3 @@ node_modules/test-dep-a To address all issues, run: npm audit fix ` - -exports[`test/lib/commands/audit.js TAP signature verification with valid signatures > must match snapshot 1`] = ` -verified signatures, audited 1 packages in xxx - -` diff --git a/test/lib/commands/audit.js b/test/lib/commands/audit.js index b28805c005871..3a880f6471d78 100644 --- a/test/lib/commands/audit.js +++ b/test/lib/commands/audit.js @@ -1,12 +1,13 @@ +const fs = require('fs') +const zlib = require('zlib') +const path = require('path') const t = require('tap') -const { load: loadMockNpm } = require('../../fixtures/mock-npm') +const { load: loadMockNpm, fake: mockNpm } = require('../../fixtures/mock-npm') const MockRegistry = require('../../fixtures/mock-registry.js') -const zlib = require('zlib') -const gzip = zlib.gzipSync + const gunzip = zlib.gunzipSync -const path = require('path') -const fs = require('fs') +const gzip = zlib.gzipSync t.cleanSnapshot = str => str.replace(/packages in [0-9]+[a-z]+/g, 'packages in xxx') @@ -59,42 +60,6 @@ const tree = { }, } -const validSignatureTree = { - 'package.json': JSON.stringify({ - name: 'test-dep', - version: '1.0.0', - dependencies: { - 'kms-demo': '1.0.0', - }, - }), - 'package-lock.json': JSON.stringify({ - name: 'test-dep', - version: '1.0.0', - lockfileVersion: 2, - requires: true, - packages: { - '': { - name: 'scratch', - version: '1.0.0', - dependencies: { - 'kms-demo': '^1.0.0', - }, - devDependencies: {}, - }, - 'node_modules/kms-demo': { - version: '1.0.0', - resolved: 'https://registry.npmjs.org/kms-demo/-/kms-demo-1.0.0.tgz', - }, - }, - dependencies: { - 'kms-demo': { - version: '1.0.0', - resolved: 'https://registry.npmjs.org/kms-demo/-/kms-demo-1.0.0.tgz', - }, - }, - }), -} - t.test('normal audit', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { prefixDir: tree, @@ -273,46 +238,116 @@ t.test('completion', async t => { }) }) -t.test('signature verification with valid signatures', async t => { - const { npm, joinedOutput } = await loadMockNpm(t, { - prefixDir: validSignatureTree, +t.test('audit signatures', async t => { + let npmOutput = [] + const joinedOutput = () => npmOutput.join('\n') + + const npm = mockNpm({ + prefix: t.testdirName, + config: { + global: false, + }, + flatOptions: { + workspacesEnabled: true, + }, + output: (str) => { + npmOutput.push(str) + }, }) + const registry = new MockRegistry({ tap: t, registry: npm.config.get('registry'), }) - const manifest = registry.manifest({ - name: 'kms-demo', - packuments: [{ - version: '1.0.0', - dist: { - integrity: 'sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPca' + - 'uoiDFJlGbZMFq5GDCurAGNSghJQ==', - signatures: [ - { - keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA', - sig: 'MEUCIDrLNspFeU5NZ6d55ycVBZIMXnPJi/XnI1Y2dlJvK8P1AiEAnXjn1IOMUd+U7YfPH' + - '+FNjwfLq+jCwfH8uaxocq+mpPk=', - }, - ], - }, - }], + + const mocks = { + '../../../lib/utils/reify-finish.js': () => Promise.resolve(), + } + const Audit = t.mock('../../../lib/commands/audit.js', mocks) + const audit = new Audit(npm) + + t.afterEach(() => { + npm.prefix = t.testdirName + npmOutput = [] }) - await registry.package({ manifest }) - registry.nock.get('/-/npm/v1/keys') - .reply(200, { - keys: [{ - expires: null, - keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA', - keytype: 'ecdsa-sha2-nistp256', - scheme: 'ecdsa-sha2-nistp256', - key: 'MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+' + - 'IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg==', + + t.test('signature verification with valid signatures', async t => { + npm.prefix = t.testdir({ + 'package.json': JSON.stringify({ + name: 'test-dep', + version: '1.0.0', + dependencies: { + 'kms-demo': '1.0.0', + }, + }), + node_modules: { + 'kms-demo': { + 'package.json': JSON.stringify({ + name: 'kms-demo', + version: '1.0.0', + }), + }, + }, + 'package-lock.json': JSON.stringify({ + name: 'test-dep', + version: '1.0.0', + lockfileVersion: 2, + requires: true, + packages: { + '': { + name: 'scratch', + version: '1.0.0', + dependencies: { + 'kms-demo': '^1.0.0', + }, + }, + 'node_modules/kms-demo': { + version: '1.0.0', + }, + }, + dependencies: { + 'kms-demo': { + version: '1.0.0', + }, + }, + }), + }) + + const manifest = registry.manifest({ + name: 'kms-demo', + packuments: [{ + version: '1.0.0', + dist: { + integrity: 'sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPca' + + 'uoiDFJlGbZMFq5GDCurAGNSghJQ==', + signatures: [ + { + keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA', + sig: 'MEUCIDrLNspFeU5NZ6d55ycVBZIMXnPJi/XnI1Y2dlJvK8P1AiEAnXjn1IOMUd+U7YfPH' + + '+FNjwfLq+jCwfH8uaxocq+mpPk=', + }, + ], + }, }], }) + await registry.package({ manifest }) + registry.nock.get('/-/npm/v1/keys') + .reply(200, { + keys: [{ + expires: null, + keyid: 'SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA', + keytype: 'ecdsa-sha2-nistp256', + scheme: 'ecdsa-sha2-nistp256', + key: 'MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+' + + 'IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg==', + }], + }) - await npm.exec('audit', ['signatures']) - t.ok(process.exitCode, 'would have exited uncleanly') - process.exitCode = 0 - t.matchSnapshot(joinedOutput()) + await audit.exec(['signatures']) + + t.equal(process.exitCode, 0, 'should exit successfully') + process.exitCode = 0 + t.match(joinedOutput(), /verified registry signatures, audited 1 packages/) + t.matchSnapshot(joinedOutput()) + }) })