Skip to content
This repository has been archived by the owner. It is now read-only.

Adds --parseable parameter to audit #20554

Merged
merged 8 commits into from Jul 10, 2018
Merged

Adds --parseable parameter to audit #20554

merged 8 commits into from Jul 10, 2018

Conversation

@luislobo
Copy link
Contributor

@luislobo luislobo commented May 9, 2018

How and why the current situation is problematic: npm is too verbose for purposes like doing a `| grep critical' and having real useful information for automated tools

By adding a new --list parameter this situation can be handled, being also an optional one. For a detailed list of changes/updates needed, there still always is the default audit command.

Use cases: when automating security audits, the result can easily be parsed by any tool.

Any caveats: Needs npm/npm-audit-report#10. There is a TODO comment in that PR that needs to be answered/addressed.

@luislobo luislobo requested a review from as a code owner May 9, 2018
@luislobo
Copy link
Contributor Author

@luislobo luislobo commented May 9, 2018

@zkat Could you help me here? I don't know where you define the extra params so that the unit test passes...

@legodude17
Copy link
Contributor

@legodude17 legodude17 commented May 9, 2018

If I understand this right, you need to put some documentation about it here, base it off of another file in that directory. Also here add it to defaults and types.

@zkat
Copy link
Contributor

@zkat zkat commented May 9, 2018

@luislobo so since this is meant to be greppable, what about using --parseable?

Try npm install --parseable -- I wonder if this is more what you'd like, instead of making something that looks like a human-readable view blow right past 80col?

@luislobo
Copy link
Contributor Author

@luislobo luislobo commented May 9, 2018

@zkat I just checked --parseable, I like the idea. I could actually add both, --list and --parseable, as I can see the use of both. The current one as a "simple" view of the default one, and the parseable one only for tooling/automation. What do you think?

@luislobo
Copy link
Contributor Author

@luislobo luislobo commented May 9, 2018

I was also thinking of making --list columns configurable, so you can add/remove columns, but that was kind of a stretch goal after the basic implementation is done

…list of results, with no titles or summaries. Allows for colors. Added docs
@luislobo
Copy link
Contributor Author

@luislobo luislobo commented May 14, 2018

@zkat @evilpacket I've just updated both npm/npm-audit-report#10 and here to match what you have suggested. I hope this lines up with what might be a good solution for everyone out there.

@luislobo
Copy link
Contributor Author

@luislobo luislobo commented May 14, 2018

Current output looks like these, of course it goes out of the bounds of 80 chars, but as a parseable solution, it's acceptable. I've been able to grep it, awk it and it's great so far

2018-05-13-20-45-23-screenshot

@luislobo
Copy link
Contributor Author

@luislobo luislobo commented May 14, 2018

Finally, one thing that I did, is group by severity, so that the output is sorted by High, Moderate, and Low

Copy link
Contributor

@iarna iarna left a comment

While we're adding --parseable we should add --json as well. There's already a npm-audit-report JSON reporter so adding it should be very little work.

To land this will need to have passing tests. The failure on Node 6 can be fixed be rebasing on to the current version of the branch. The other issues seem to be formatting issues that are making our linter unhappy.

@legodude17
Copy link
Contributor

@legodude17 legodude17 commented May 15, 2018

@iarna --json was already added to npm audit by #20568.

@luislobo
Copy link
Contributor Author

@luislobo luislobo commented May 16, 2018

@legodude17 One thing I noticed about #20568. is that it doesn't add Docs or help. I'll add it in this branch if you all are OK.

@legodude17
Copy link
Contributor

@legodude17 legodude17 commented May 16, 2018

@luislobo Not my call to make

@luislobo luislobo closed this May 22, 2018
@luislobo luislobo deleted the audit-list branch May 22, 2018
@luislobo luislobo restored the audit-list branch May 22, 2018
@luislobo luislobo reopened this May 22, 2018
@luislobo
Copy link
Contributor Author

@luislobo luislobo commented May 22, 2018

@iarna I fixed the standard issues. Should be good to go, AFAIK.

@luislobo luislobo changed the title Adds --list parameter to audit Adds --parseable parameter to audit May 28, 2018
@luislobo luislobo closed this Jun 1, 2018
@luislobo luislobo reopened this Jun 1, 2018
Remove unused variable
@luislobo
Copy link
Contributor Author

@luislobo luislobo commented Jun 6, 2018

This param needs npm/npm-audit-report#21

@luislobo
Copy link
Contributor Author

@luislobo luislobo commented Jun 6, 2018

@zkat Any chance this can be added to next releases? We are using it internally and really saves time checking all our modules (have more than 40) but we have to keep updating whenever there's a new npm version.

Thanks!

@luislobo
Copy link
Contributor Author

@luislobo luislobo commented Jun 12, 2018

@zkat @iarna I don't want to be pushy, may be I just need to follow some process that I'm not aware of, but, how does a PR get into the list of PRs to be evaluated as one that might get into a release?

@luislobo
Copy link
Contributor Author

@luislobo luislobo commented Jul 9, 2018

@zkat any chance to get this one reviewed?

@zkat
Copy link
Contributor

@zkat zkat commented Jul 9, 2018

I got around to merging all the PRs on npm-audit-report, so this should be unblocked. /cc @iarna

@luislobo
Copy link
Contributor Author

@luislobo luislobo commented Jul 10, 2018

Thanks @zkat. @iarna let me know if there are any changes needed here

zkat
zkat approved these changes Jul 10, 2018
Copy link
Contributor

@zkat zkat left a comment

we're good, I think! Thank you!

@zkat zkat changed the base branch from latest to release-next Jul 10, 2018
@zkat zkat merged this pull request into npm:release-next Jul 10, 2018
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
zkat added a commit that referenced this issue Jul 10, 2018
@luislobo luislobo deleted the audit-list branch Jul 10, 2018
@luislobo luislobo restored the audit-list branch Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants