From 3ef295f23ee1b2300abf13ec19e935c47a455179 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 15 Nov 2019 11:28:19 -0800 Subject: [PATCH] fix: print quick audit report for human output 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: https://github.com/npm/cli/pull/486 Credit: @isaacs Close: #486 Reviewed-by: @claudiahdz --- lib/install.js | 7 +- lib/install/fund.js | 4 +- test/tap/audit.js | 120 +++++++++++++++++++++++++++- test/tap/install-mention-funding.js | 14 ++-- test/tap/install.fund.js | 19 +++-- 5 files changed, 141 insertions(+), 23 deletions(-) diff --git a/lib/install.js b/lib/install.js index 528a3a0b59a75..378ada7b05c06 100644 --- a/lib/install.js +++ b/lib/install.js @@ -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' : '') @@ -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) { diff --git a/lib/install/fund.js b/lib/install/fund.js index 55a167a95583a..809c05b33878b 100644 --- a/lib/install/fund.js +++ b/lib/install/fund.js @@ -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 }) { diff --git a/test/tap/audit.js b/test/tap/audit.js index 631eedf276e37..ca3da87a3af62 100644 --- a/test/tap/audit.js +++ b/test/tap/audit.js @@ -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({ @@ -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': { @@ -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: [{ @@ -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({ diff --git a/test/tap/install-mention-funding.js b/test/tap/install-mention-funding.js index ebd25a57324c1..3e9b81f24070b 100644 --- a/test/tap/install-mention-funding.js +++ b/test/tap/install-mention-funding.js @@ -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() }) }) @@ -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() }) }) @@ -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() }) }) @@ -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() }) }) diff --git a/test/tap/install.fund.js b/test/tap/install.fund.js index 37a61e42891af..fca5fb3afd123 100644 --- a/test/tap/install.fund.js +++ b/test/tap/install.fund.js @@ -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', @@ -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: { @@ -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: { @@ -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: { @@ -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()