-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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.
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
core(vulnerable-libs): add fix for recovering from bad versions #3932
Conversation
}) | ||
|
||
it('should fix bad version numbers', () => { | ||
assert.equal(NoVulnerableLibrariesAudit.normalizeVersion('11.51'), '11.51.0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want it to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seemed reasonable to me as there were a lot in sentry, but I'm not sure any libraries that we for which we get just major.minor
actually have a corresponding npm package
I'm fine to revert if you think its bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah its fine
Sentry.captureException(err, {level: 'warning'}); | ||
return vulns; | ||
} | ||
|
||
lib.pkgLink = 'https://snyk.io/vuln/npm:' + lib.npmPkgName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do me a favor and update this line to
lib.pkgLink = `https://snyk.io/vuln/npm:${lib.npmPkgName}#lh@${lib.version}`;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh done
@@ -62,6 +82,15 @@ class NoVulnerableLibrariesAudit extends Audit { | |||
return vulns; | |||
} | |||
|
|||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idea: instead of try/catch we could make another call to semver.valid()
. i don't care much either way. you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well I wanted to reuse the error object it generates to beacon to sentry, but I suppose not the end of the world to create our own with captureMessage
instead?
I'm don't feel particularly strongly either way
fixes #3624