Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add npm audit licenses #3452

Closed
wants to merge 7 commits into from
Closed

feat: add npm audit licenses #3452

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1432cca
feat: add license checking with json output
bnb Jun 22, 2021
3075d72
test: test for license checking with json output
bnb Jun 22, 2021
46a6471
deps: move licensee to dep rather than devDep
bnb Jun 22, 2021
d88b262
chore: minor style changes
bnb Sep 7, 2021
f3e183d
feat: some more work on Audit
bnb Sep 9, 2021
73aed2b
chore: add licenses to allow in license check
bnb Sep 9, 2021
9b2cd1e
Merge branch 'latest' into npm-audit-licenses
bnb Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 53 additions & 2 deletions lib/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const Arborist = require('@npmcli/arborist')
const auditReport = require('npm-audit-report')
const licensee = require('licensee')
const reifyFinish = require('./utils/reify-finish.js')
const auditError = require('./utils/audit-error.js')
const ArboristWorkspaceCmd = require('./workspaces/arborist-cmd.js')
Expand Down Expand Up @@ -48,10 +49,15 @@ class Audit extends ArboristWorkspaceCmd {
}

exec (args, cb) {
this.audit(args).then(() => cb()).catch(cb)
const auditType = this.npm.config.get('audit-type')
if (auditType === 'license') {
this.auditLicenses(args).then(() => cb()).catch(cb)
} else {
this.auditAdvisories(args).then(() => cb()).catch(cb)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean that npm audit can only do either licenses or advisories, but not both?

Should audit-type be validated somehow so it's only "license" or "advisory"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb in pairing with @ruyadorno today, we intentionally kept this limited to "getting it working" and not fully implementing. This was the minimum way to get it working. There will be individual ways to do advisories, licenses, and both (presuming both will be the default, since license is going to be opt-in rather than opt-out).

Ruy's recommendation was npm audit --audit-type=license as a more npm-y API, so we went with that. I'm not opposed to other APIs though and presume we'll end up bikeshedding a bit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I just want one type, that totally makes sense to me - but when I want both, I'd want some config way to per-command override the global audit-type=license i plan to configure.

As for the default, I'd assume licenses aren't validated unless there's a config for it, which means it'd be off initially?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oop, sorry thought I replied to this last night:

the intent is to have npm audit do both licenses and advisories by default. It can optionally do just licenses or advisories.

the implementation here is an intentionally basic one that's the result of @ruyadorno and myself pairing trying to get a very basic implementation working that can be expanded on to include everything that's been scoped out, hence WIP status of the PR.

Copy link
Contributor

@ljharb ljharb Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - so what audit-type value would I provide to override (a global config setting that restricts it to "just licenses")?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

advisories: https://github.com/npm/cli/pull/3452/files#diff-2a8ed1f0d31e4c759861aa941a2dc7e4965c10d96a79bad6499301a9a0dadbc2R56

}

async audit (args) {
async auditAdvisories (args) {
const reporter = this.npm.config.get('json') ? 'json' : 'detail'
const opts = {
...this.npm.flatOptions,
Expand All @@ -74,6 +80,51 @@ class Audit extends ArboristWorkspaceCmd {
this.npm.output(result.report)
}
}

async auditLicenses (args) {
const reporter = this.npm.config.get('json') ? 'json' : 'detail'
const config = { // this is extremely limited and should consume user input rather than our own config
"licenses": {
"spdx": [
"MIT"
]
}
}
await new Promise((res, rej) => {
const licensesPath = this.npm.prefix
licensee(config, licensesPath, (error, dependencies) => {
if (error) rej(error)
if (reporter === "json") {
this.jsonStringifyOutput(dependencies)
}
res()
})
})

}

jsonStringifyOutput (dependencies) {
const loggableProperties = [
'name',
'version',
'approved',
'license',
'corrected',
'repository',
'homepage',
'author',
'contributors'
]

const out = {}
for(const dep of dependencies) {
out[dep.name] = {}
for (const prop of loggableProperties) {
out[dep.name][prop] = dep[prop]
}
}
bnb marked this conversation as resolved.
Show resolved Hide resolved
this.npm.output(JSON.stringify(out, null, 2))
}
}

module.exports = Audit
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
"libnpmsearch": "^3.1.1",
"libnpmteam": "^2.0.3",
"libnpmversion": "^1.2.1",
"licensee": "^8.2.0",
"make-fetch-happen": "^9.0.3",
"minipass": "^3.1.3",
"minipass-pipeline": "^1.2.4",
Expand Down Expand Up @@ -196,7 +197,6 @@
"eslint-plugin-node": "^11.1.0",
"eslint-plugin-promise": "^5.1.0",
"eslint-plugin-standard": "^5.0.0",
"licensee": "^8.2.0",
"tap": "^15.0.9"
},
"scripts": {
Expand Down
48 changes: 48 additions & 0 deletions test/lib/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,54 @@ t.test('report endpoint error', t => {
t.end()
})

t.test('licenses', t => {
const Audit = require('../../lib/audit.js')
t.test('run audit with license type and output json', t => {
const prefix = t.testdir({
node_modules: {
a: {
'package.json': JSON.stringify({
name: 'a',
version: '1.0.0',
license: 'MIT',
}),
},
b: {
'package.json': JSON.stringify({
name: 'b',
version: '1.0.0',
license: 'ISC',
}),
},
},
})

const OUTPUT = []
const npm = mockNpm({
prefix: prefix,
command: 'audit',
config: {
"audit-type": "license",
json: true
},
output: (...msg) => {
OUTPUT.push(msg)
},
})

const audit = new Audit(npm)

audit.exec([], () => {
t.strictSame(JSON.parse(OUTPUT), {
a: { name: 'a', version: '1.0.0', approved: true, license: 'MIT' },
b: { name: 'b', version: '1.0.0', approved: false, license: 'ISC' }
})
t.end()
})
})
t.end()
})

t.test('completion', t => {
const Audit = require('../../lib/audit.js')
const audit = new Audit({})
Expand Down