Skip to content

Commit

Permalink
Add a working test
Browse files Browse the repository at this point in the history
  • Loading branch information
@feelepxyz
feelepxyz committed May 12, 2022
1 parent 054b5fe commit 0def984
Show file tree
Hide file tree
Showing 3 changed files with 185 additions and 174 deletions.
166 changes: 71 additions & 95 deletions lib/commands/audit.js
View file
Expand Up @@ -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 }) => {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down
10 changes: 5 additions & 5 deletions tap-snapshots/test/lib/commands/audit.js.test.cjs
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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
`

0 comments on commit 0def984

Please sign in to comment.