Add comments to decisions

[xavierlefevre]

Hello guys, discovered your lib thanks to @clement-escolano, works greatly.

While resolving my security warnings, and ignoring some, I thought that allowing to add a comment could be useful, to remember the why of some decisions.

Is it a little something you might find useful and planned to add?

[naugtur]

I was thinking about requesting a reason when ignoring, but didn't come up with the interaction I'd want to be there. I don't want to slow down the default process of making decisions. Maybe a rule that'd turn it on it require... Share your thoughts pls

[RichardBradley]

I was surprised to not get asked for a reason comment when "ignoring" an audit vulnernability.

I was thinking about requesting a reason when ignoring, but didn't come up with the interaction I'd want to be there. I don't want to slow down the default process of making decisions.

I would expect the "ignore" action to prompt for a "reason" comment, but to allow empty comments. I don't think speed of ignoring audit vulnerabilities is the most important factor here :-)

Thanks for a great tool

[zcabjro]

We've taken to manually adding a comment to audit-resolve.json (e.g. "ignoreComment": "...") that typically points to relevant GitHub issues or otherwise explains the decision and why we ignore for 1 week instead of 1 month etc.

Having this built-in to the ignore flow would be great!

[naugtur]

I promise I'll add it eventually. It's been a thing even before this thread. Currently I'm focusing the little time I have on getting npm to support the audit resolve file directly.

[ronjouch]

I don't want to slow down the default process of making decisions. Maybe a rule that'd turn it on it require... Share your thoughts pls

@naugtur thanks for listening; here are my thoughts. I'd like this too, and yeah I feel too it would make sense to have this optional (for backwards compat), but well-publicized in README & release notes. It could be behind a command-line flag --require-reason to both commands:

1. check-audit --require-reason would require a "reason" key for each item of the "decisions" array.
   1. Lack a reason would cause check-audit to: 1. stderr a helpful error message (something like "No reason was specified for ignored vulnerability <xyz>. Please specify one, either using 'resolve-audit --require-reason' or by editing resolve-audit.json manually), and 2. exit with a non-0 error code.
   2. --require-reason should minimally be a boolean option, but it'd be nice to support passing a regex. For example, I'd like to set a regex like /(inapplicable-to-our-code|postponed-for-now|devdep-and-verified-as-not-a-supply-chain-attack) I confirm that I took the time to understand the vuln. Yes it can be safely ignored./ . The (categoryA|categoryB|...) group would force categorization of ignored vulns, to force devs into thinking about the ignore they're creating (discouraging over-the-shoulder ignoring). And adding to the regex the requirement of a formal "I confirm ..." sentence is extra pressure to encourage being careful with this.
2. resolve-audit --require-reason would support interactively prompting for a reason, and persisting it to resolve-audit.json.
   1. And @naugtur to echo your comment "[I] didn't come up with the interaction I'd want to be there": zero interaction is fine by me. We can start with the feature in check-audit only and postponing interactivity in resolve-audit to later, both things don't have to ship at the same time. What matters to me is to have this feature in check-audit; asking devs to manually add a "reason" to in a JSON file is fine, and that's what some of us do already (see comment below).
   2. On that note, @naugtur would you take a PR adding the check-audit side? I might be a low-hanging fruit quick to implement, that I can probably do for $JOB.

We've taken to manually adding a comment to audit-resolve.json (e.g. "ignoreComment": "...") that typically points to relevant GitHub issues or otherwise explains the decision [...].

Same here.

[naugtur]

I'll get back to this topic soon.

Trying to figure out if I want to iron out issues with v3 and get your suggestions built on top of that, or do a major rewrite.

Using npm and yarn commands as backend is annoying and limiting and I'm thinking of rebuilding.on top of arborist (the lib behind most npm CLI features) and with a different usage pattern.

[naugtur]

I pushed forward with the current progress on v3.

The reason field is implemented in core https://github.com/naugtur/audit-resolve-core/blob/master/auditFile/versions/v1.js#L20

@ronjouch Feel free to start working on the PR. Let me know if you want/can start any time soon. I might be available to get on zoom or something and give you an intro or answer questions. Branch off of npm7-dev branch.

I'd go for rules.requireReason as the configuration option instead of the commandline argument, because by default all arguments are passed down to npm or yarn so I want to avoid possible name conflicts.

[naugtur]

@ronjouch I'm going to try and finalize the v3. Do you want to work on this now to get it in the release?

[ronjouch]

I'm going to try and finalize the v3. Do you want to work on this now to get it in the release?

@naugtur I want to! But $job always throws stuff at you, causing endless postponing in favor of more urgent concerns :shrug: . But thanks for the reminder, scheduling it for our next "hack day".

Thus, for sure don't delay the final v3 release waiting for this, consider this a "potential-contribution". Anyway, this can ship in a semver-minor (extra feature) version 3.1 or 3.2, right?

[ronjouch]

I'm going to try and finalize the v3.

@naugtur also by the way, here's one more confirmation that 3.0.0-7 has been working great at work, both on npm {6, 8} workflows, in a dozen projects. No issue to report, feels ready to ship.

EDIT June 29 WIP in https://github.com/naugtur/audit-resolve-core/pull/5 / https://github.com/naugtur/npm-audit-resolver/pull/61

[naugtur]

Anyway, this can ship in a semver-minor (extra feature) version 3.1 or 3.2, right?

Would look good on the v3 announcement, but this is fine :)

[niklasholm]

Great work guys, much appreciated.

[mikethea1]

Seems like there was a good amount of momentum behind it but it died out in 2022. Any chance this will be added when 3.0 stable comes out?