Fix Yarn Audit findings bug and patch-bump version from 2.6.0 -> 2.6.1 #70
Conversation
| @@ -63,7 +63,7 @@ def run | |||
| # For all advisories, | |||
| prod = raw_advisories_for_id.any? do |raw_advisory| | |||
| # any there there any instances in the prod dependency tree? | |||
| raw_advisory.fetch(:findings).any? { |finding| !finding.fetch(:dev) } | |||
| raw_advisory.fetch(:findings).any? { |finding| !finding.fetch(:dev, false) } | |||
There was a problem hiding this comment.
is the fix to not run development dependencies?
There was a problem hiding this comment.
No, previously this scanner expected the advisories fetched by YarnAudit to have a dev key in each object of their findings array. Some advisories don't have the dev key and cause the scanner to error on this line.
The value of the dev key is typically true or false, and this change treats the value as false if the key is not provided.
TLDR: It treats the finding as a non-dev finding by default if not already specified. @xolaz
There was a problem hiding this comment.
Interesting. Can you write a quick test for this?
There was a problem hiding this comment.
The test can check that our advisory output is as expected even without the dev key. Aka, advisory output reports regardless of whether the dev key exists.
There was a problem hiding this comment.
And what does the dev key mean for NPM/Yarn? That the finding is related to a development package? It looks like this change won't report development dependencies.
There was a problem hiding this comment.
@xolaz I'm unable to find documentation related to the shape of these advisory objects and the meaning of their keys. Comments in the code claim that the key lets us know whether advisories are in some production dependency tree, but it's unclear how the scanner would know that and what the difference between the production dependency tree and trees in other staging environments would be, if any.
There was a problem hiding this comment.
Drafting a test case right now.
There was a problem hiding this comment.
Also, I spoke with @nishils and he said that the difference between production and development advisories is that prod advisories are advisories related dependencies and dev advisories are related to devDependencies. So, the dev key lets us know whether the finding is related to a dev dependency. @xolaz
There was a problem hiding this comment.
So, in essence if we're unsure whether a finding is related to a dev or non-dev dependency (no dev key provided), we assume the worst by treating it as if it's related to a non-dev dependency. @xolaz
There was a problem hiding this comment.
You can also quickly test this @geraldnash-cb by running yarn audit --json to see what the output looks like.
|
This is a good bug fix! Can we also add an option to exclude dev dependencies? |
type=nonroutine
risk=low
impact=sev5