Skip to content

Commit

Permalink
fix: print quick audit report for human output
Browse files Browse the repository at this point in the history 
This was broken when the support/funding functionality changed the
return value to no longer track the promise for the quick audit
printing.

It was not caught by tests, because they were only running against the
--json output, and not verifying the quick audit results in any way.

Added a test to track the --json quick audit results (which were not
broken, but someday could become so) and the human printed quick audit
results (which were broken).

Paired with @ruyadorno @mikemimik

PR-URL: #486
Credit: @isaacs
Close: #486
Reviewed-by: @claudiahdz
  • Loading branch information
@isaacs @claudiahdz
isaacs authored and claudiahdz committed Nov 18, 2019
1 parent b150eae commit 3ef295f
Show file tree
Hide file tree
Showing 5 changed files with 141 additions and 23 deletions.
7 changes: 4 additions & 3 deletions lib/install.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -878,9 +878,6 @@ Installer.prototype.printInstalledForHuman = function (diffs, auditResult) {
report += ' in ' + ((Date.now() - this.started) / 1000) + 's'

output(report)
if (auditResult) {
audit.printInstallReport(auditResult)
}

function packages (num) {
return num + ' package' + (num > 1 ? 's' : '')
Expand Down Expand Up @@ -911,6 +908,10 @@ Installer.prototype.printInstalledForHuman = function (diffs, auditResult) {
if (printFundingReport.length) {
output(printFundingReport)
}

if (auditResult) {
return audit.printInstallReport(auditResult)
}
}

Installer.prototype.printInstalledForJSON = function (diffs, auditResult) {
Expand Down
4 changes: 2 additions & 2 deletions lib/install/fund.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ function getPrintFundingReport ({ fund, idealTree }, opts) {

return padding('') + length + ' ' +
packageQuantity(length) +
' looking for funding.' +
padding('Run "npm fund" to find out more.')
' looking for funding' +
padding(' run `npm fund` for details\n')
}

function getPrintFundingReportJSON ({ fund, idealTree }) {
Expand Down
120 changes: 119 additions & 1 deletion test/tap/audit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,66 @@ function tmock (t) {
})
}

const quickAuditResult = {
actions: [],
advisories: {
'1316': {
findings: [
{
version: '1.0.0',
paths: [
'baddep'
]
}
],
'id': 1316,
'created': '2019-11-14T15:29:41.991Z',
'updated': '2019-11-14T19:35:30.677Z',
'deleted': null,
'title': 'Arbitrary Code Execution',
'found_by': {
'link': '',
'name': 'François Lajeunesse-Robert',
'email': ''
},
'reported_by': {
'link': '',
'name': 'François Lajeunesse-Robert',
'email': ''
},
'module_name': 'baddep',
'cves': [],
'vulnerable_versions': '<4.5.2',
'patched_versions': '>=4.5.2',
'overview': 'a nice overview of the advisory',
'recommendation': 'how you should fix it',
'references': '',
'access': 'public',
'severity': 'high',
'cwe': 'CWE-79',
'metadata': {
'module_type': '',
'exploitability': 6,
'affected_components': ''
},
'url': 'https://npmjs.com/advisories/1234542069'
}
},
'muted': [],
'metadata': {
'vulnerabilities': {
'info': 0,
'low': 0,
'moderate': 0,
'high': 1,
'critical': 0
},
'dependencies': 1,
'devDependencies': 0,
'totalDependencies': 1
}
}

test('exits with zero exit code for vulnerabilities below the `audit-level` flag', t => {
const fixture = new Tacks(new Dir({
'package.json': new File({
Expand All @@ -40,7 +100,7 @@ test('exits with zero exit code for vulnerabilities below the `audit-level` flag
fixture.create(testDir)
return tmock(t).then(srv => {
srv.filteringRequestBody(req => 'ok')
srv.post('/-/npm/v1/security/audits/quick', 'ok').reply(200, 'yeah')
srv.post('/-/npm/v1/security/audits/quick', 'ok').reply(200, quickAuditResult)
srv.get('/baddep').twice().reply(200, {
name: 'baddep',
'dist-tags': {
Expand Down Expand Up @@ -75,6 +135,8 @@ test('exits with zero exit code for vulnerabilities below the `audit-level` flag
'--registry', common.registry,
'--cache', path.join(testDir, 'npm-cache')
], EXEC_OPTS).then(([code, stdout, stderr]) => {
const result = JSON.parse(stdout)
t.same(result.audit, quickAuditResult, 'printed quick audit result')
srv.filteringRequestBody(req => 'ok')
srv.post('/-/npm/v1/security/audits', 'ok').reply(200, {
actions: [{
Expand Down Expand Up @@ -102,6 +164,62 @@ test('exits with zero exit code for vulnerabilities below the `audit-level` flag
})
})

test('shows quick audit results summary for human', t => {
const fixture = new Tacks(new Dir({
'package.json': new File({
name: 'foo',
version: '1.0.0',
dependencies: {
baddep: '1.0.0'
}
})
}))
fixture.create(testDir)
return tmock(t).then(srv => {
srv.filteringRequestBody(req => 'ok')
srv.post('/-/npm/v1/security/audits/quick', 'ok').reply(200, quickAuditResult)
srv.get('/baddep').twice().reply(200, {
name: 'baddep',
'dist-tags': {
'latest': '1.2.3'
},
versions: {
'1.0.0': {
name: 'baddep',
version: '1.0.0',
_hasShrinkwrap: false,
dist: {
shasum: 'deadbeef',
tarball: common.registry + '/idk/-/idk-1.0.0.tgz'
}
},
'1.2.3': {
name: 'baddep',
version: '1.2.3',
_hasShrinkwrap: false,
dist: {
shasum: 'deadbeef',
tarball: common.registry + '/idk/-/idk-1.2.3.tgz'
}
}
}
})
return common.npm([
'install',
'--audit',
'--no-json',
'--package-lock-only',
'--registry', common.registry,
'--cache', path.join(testDir, 'npm-cache')
], EXEC_OPTS).then(([code, stdout, stderr]) => {
t.match(stdout, new RegExp('added 1 package and audited 1 package in .*\\n' +
'found 1 high severity vulnerability\\n' +
' run `npm audit fix` to fix them, or `npm audit` for details\\n'),
'shows quick audit result')
})
})
})

test('exits with non-zero exit code for vulnerabilities at the `audit-level` flag', t => {
const fixture = new Tacks(new Dir({
'package.json': new File({
Expand Down
14 changes: 7 additions & 7 deletions test/tap/install-mention-funding.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ test('mention npm fund upon installing single dependency', function (t) {
if (err) throw err
t.is(code, 0, 'installed successfully')
t.is(stderr, '', 'no warnings')
t.includes(stdout, '1 package is looking for funding.', 'should print amount of packages needing funding')
t.includes(stdout, 'Run "npm fund" to find out more.', 'should print npm fund mention')
t.includes(stdout, '1 package is looking for funding', 'should print amount of packages needing funding')
t.includes(stdout, ' run `npm fund` for details', 'should print npm fund mention')
t.end()
})
})
Expand All @@ -80,8 +80,8 @@ test('mention npm fund upon installing multiple dependencies', function (t) {
if (err) throw err
t.is(code, 0, 'installed successfully')
t.is(stderr, '', 'no warnings')
t.includes(stdout, '4 packages are looking for funding.', 'should print amount of packages needing funding')
t.includes(stdout, 'Run "npm fund" to find out more.', 'should print npm fund mention')
t.includes(stdout, '4 packages are looking for funding', 'should print amount of packages needing funding')
t.includes(stdout, ' run `npm fund` for details', 'should print npm fund mention')
t.end()
})
})
Expand All @@ -92,8 +92,8 @@ test('skips mention npm fund using --no-fund option', function (t) {
if (err) throw err
t.is(code, 0, 'installed successfully')
t.is(stderr, '', 'no warnings')
t.doesNotHave(stdout, '4 packages are looking for funding.', 'should print amount of packages needing funding')
t.doesNotHave(stdout, 'Run "npm fund" to find out more.', 'should print npm fund mention')
t.doesNotHave(stdout, '4 packages are looking for funding', 'should print amount of packages needing funding')
t.doesNotHave(stdout, ' run `npm fund` for details', 'should print npm fund mention')
t.end()
})
})
Expand All @@ -105,7 +105,7 @@ test('mention packages looking for funding using --json', function (t) {
t.is(code, 0, 'installed successfully')
t.is(stderr, '', 'no warnings')
const res = JSON.parse(stdout)
t.match(res.funding, '4 packages are looking for funding.', 'should print amount of packages needing funding')
t.match(res.funding, '4 packages are looking for funding', 'should print amount of packages needing funding')
t.end()
})
})
Expand Down
19 changes: 9 additions & 10 deletions test/tap/install.fund.js
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
'use strict'

const { EOL } = require('os')
const { test } = require('tap')
const { getPrintFundingReport } = require('../../lib/install/fund')

test('message when there are no funding found', (t) => {
t.deepEqual(
t.equal(
getPrintFundingReport({}),
'',
'should not print any message if missing info'
)
t.deepEqual(
t.equal(
getPrintFundingReport({
name: 'foo',
version: '1.0.0',
Expand All @@ -19,7 +18,7 @@ test('message when there are no funding found', (t) => {
'',
'should not print any message if package has no dependencies'
)
t.deepEqual(
t.equal(
getPrintFundingReport({
fund: true,
idealTree: {
Expand All @@ -38,7 +37,7 @@ test('message when there are no funding found', (t) => {
})

test('print appropriate message for a single package', (t) => {
t.deepEqual(
t.equal(
getPrintFundingReport({
fund: true,
idealTree: {
Expand All @@ -54,15 +53,15 @@ test('print appropriate message for a single package', (t) => {
}
]
}
}),
`${EOL}1 package is looking for funding.${EOL}Run "npm fund" to find out more.`,
}).replace(/[\r\n]+/g, '\n'),
`\n1 package is looking for funding\n run \`npm fund\` for details\n`,
'should print single package message'
)
t.end()
})

test('print appropriate message for many packages', (t) => {
t.deepEqual(
t.equal(
getPrintFundingReport({
fund: true,
idealTree: {
Expand Down Expand Up @@ -92,8 +91,8 @@ test('print appropriate message for many packages', (t) => {
}
]
}
}),
`${EOL}3 packages are looking for funding.${EOL}Run "npm fund" to find out more.`,
}).replace(/[\r\n]+/g, '\n'),
`\n3 packages are looking for funding\n run \`npm fund\` for details\n`,
'should print many package message'
)
t.end()
Expand Down

0 comments on commit 3ef295f

Please sign in to comment.