tests(snyk): assert upper bounds #9308

connorjclark · 2019-06-28T22:18:57Z

Since we ship to places we can't necessarily quickly update (e.g. DevTools), I wonder if we should have a LH test that fails if the database has * or >=X, and require it be corrected to <= some hardcoded version (presumably the latest available release), even if that version is still vulnerable.

The theory would be: if a new, still vulnerable version of the library comes out after that LH release, then it's better to have a false negative (given that there's no fix available anyways and there are other sources (github, npm, greenkeeper, etc) that will warn folks) than it is to have up to 12 weeks of a false positive (if a new, non-vulnerable version comes out) which would annoy folks and encourage them to ignore Lighthouse's warnings in the future.

Should <=X be added to the end of every semver.vulnerable, where X = the latest release within that major version?

patrickhulce · 2019-06-28T22:22:25Z

Yeah I think we need to be stronger than no * to satisfy this, maybe nothing * and no unqualified >, i.e. all < or explicit ranges

connorjclark · 2019-06-28T22:25:08Z

We'll need a process for anything more than just no * please. need to augment the semvers that don't have an upper bound.

When snyk does its updating (the PR it auto-magically opens), is there a way for us to hook into it and make our changes on top?

brendankenny · 2019-06-28T22:30:44Z

Yes, there's a script in our repo they run to trim it down, would be a good place for this.

brendankenny · 2019-06-28T22:32:08Z

https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/scripts/cleanup-vuln-snapshot.js

connorjclark · 2019-06-28T22:35:25Z

{"id":"npm:moment:20170905","severity":"low","semver":{"vulnerable":["<2.19.3"]}}

{"id":"npm:knockout:20130701","severity":"medium","semver":{"vulnerable":["<3.0.0 >=2.1.0-pre"]}}

{"id":"npm:jquery:20150627","severity":"medium","semver":{"vulnerable":["<1.12.2",">=1.12.3 <2.2.2",">=2.2.3 <3.0.0"]}},

Every entry has an upper bound - good.

{"id":"npm:dojo:20100614","severity":"high","semver":{"vulnerable":[">=0.4 <0.4.4 || >=1.0 <1.0.3 || >=1.1 <1.1.2 || >=1.2 <1.2.4 || >=1.3 <1.3.3 || >=1.4 <1.4.2"]}},

is a hard one to grok (and luckily each clause is already given an upperbound)

I actually thought there would be some entries with no upperbound, but I didn't see any. So let's just add the assertion that there isn't one, and address it if it ever gets triggered.

patrickhulce

LGTM

patrickhulce · 2019-07-08T22:21:19Z

lighthouse-core/test/audits/dobetterweb/no-vulnerable-libraries-test.js

+
+        const clauses = semver.split('||');
+        for (const clause of clauses) {
+          if (!clause.trim().startsWith('=') && !clause.includes('<')) {


maybe give a few examples here for easier parsing? I had to go checkout the snapshot to see why it's includes('<') but .trim().startsWith('=')

I had to go checkout the snapshot to see why it's includes('<') but .trim().startsWith('=')

hmm, I still don't understand why that's the case :).

Isn't something like '=5.0 >=10' valid with no upper bound? But it wouldn't fail this check. Or '<5.0 >=10'? Maybe I just don't understand what we're checking here :)

well those aren't real ranges :)

I suppose it's a meta question of if we should be checking to see that they're valid ranges at all

well those aren't real ranges :)

why aren't they? Isn't '=5.0 >=10' just saying 5.x or anything over 10 is vulnerable?

well those aren't real ranges :)

this

valid ranges at all

doesn't make sense to validate snyk like that imo

a space is an AND

OR is either || or a separate entry in the vulnerable array

so while it might technically be valid, nothing will ever satisfy the range '=5 >=10'

semver package's validRange just verifies the string conforms to the grammar defined here: https://github.com/npm/node-semver/blob/ce6190e2b681700dcc5d7309fe8eda99941f712d/range.bnf not that the range is satisfiable.

so while it might technically be valid, nothing will ever satisfy the range '=5 >=10'

Ah, I see your point, but it could also be '=11 >= 10'. I assume most of these are auto generated, so it doesn't seem out of the realm of possibility. See my other comment for one possibility rather than getting clever and hoping these ranges always make sense :)

Ah, I see your point, but it could also be '=11 >= 10'

oh, no, wait, that's wrong. So I'll have to fall back to hand waving about invalid ranges :)

doesn't make sense to validate snyk like that imo

didnt realize how easy it'd be to use semver to validate the ranges, sooo nvm. it's done indirectly with this test now.

lighthouse-core/test/audits/dobetterweb/no-vulnerable-libraries-test.js

brendankenny · 2019-07-08T23:43:31Z

lighthouse-core/test/audits/dobetterweb/no-vulnerable-libraries-test.js

+
+        const clauses = semver.split('||');
+        for (const clause of clauses) {
+          if (!clause.trim().startsWith('=') && !clause.includes('<')) {


I had to go checkout the snapshot to see why it's includes('<') but .trim().startsWith('=')

hmm, I still don't understand why that's the case :).

Isn't something like '=5.0 >=10' valid with no upper bound? But it wouldn't fail this check. Or '<5.0 >=10'? Maybe I just don't understand what we're checking here :)

…s-test.js Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

googlebot · 2019-07-09T00:05:42Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-09T00:07:13Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

brendankenny · 2019-07-09T00:13:33Z

I guess one question would be do we strictly require an upper bound? If there's an earlier valid version, should that be sufficient?

If not, and we want to stick with requiring an upper bound, we could just use semver for this instead and do something like if (!semver.gtr('gazillions.0.0', snykRange')) assert.false(...).

brendankenny · 2019-07-09T00:26:33Z

we could just use semver for this instead and do something like if (!semver.gtr('gazillions.0.0', snykRange)) assert.false(...).

@connorjclark points out they use Number.MAX_SAFE_INTEGER, so it could be semver.gtr(`${Number.MAX_SAFE_INTEGER}.0.0`, snykRange) :)

brendankenny · 2019-07-09T00:27:22Z

(I was hoping for Math.floor(Number.MAX_VALUE) :)

connorjclark · 2019-07-09T00:38:44Z

npm/node-semver#166 (comment)

kind of like what I was doing, but normalizes the ranges w/ new semver.Range first. for example, this translates <4.1 || =5.0 into <4.1.0||>=5.0.0 <5.1.0. range.sets would have [<4.1.0] and [>=5.0.0, <5.1.0]

googlebot · 2019-07-09T01:04:07Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-09T01:05:57Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

brendankenny · 2019-07-10T18:51:24Z

lighthouse-core/test/audits/dobetterweb/no-vulnerable-libraries-test.js

+    // Upperbound exists if...
+
+    // < or <= is in one of the subset's clauses (= gets normalized to >= and <).
+    if (subset.some(comparator => comparator.operator.match(/^</))) {


I'm not sure this entirely works. A rangeString of '*' ends up throwing here, for instance.

We have an untested homegrown solution vs a tested (by semver) but ugly gtr solution...

A rangeString of '*' ends up throwing here, for instance.

Isn't that kinda desired behavior? :)

I'm not sure this entirely works. A rangeString of '*' ends up throwing here, for instance.

Fixed.

We have an untested homegrown solution vs a tested (by semver) but ugly gtr solution...

Added tests.

brendankenny · 2019-07-10T18:53:12Z

lighthouse-core/test/audits/dobetterweb/no-vulnerable-libraries-test.js

+  return true;
+}
+
+describe('every snyk vulnerability has an upper bound', () => {


needs an it() or changed to it and nested in a describe() above, otherwise on failures you end up with

googlebot · 2019-07-10T21:27:40Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-10T21:32:44Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

brendankenny

this is a net positive, even if about 27 lines longer than my really good gazillions idea :)

let's land

deps(snyk): do not allow * in semver

eafb5f9

googlebot added the cla: yes label Jun 28, 2019

assert upper bounds

a24a38e

connorjclark changed the title ~~deps(snyk): do not allow * in semver~~ deps(snyk): assert upper bounds Jun 28, 2019

vercel bot deployed to staging June 28, 2019 22:40 View deployment

connorjclark changed the title ~~deps(snyk): assert upper bounds~~ tests(snyk): assert upper bounds Jun 28, 2019

update test name

a006050

vercel bot deployed to staging June 28, 2019 22:40 View deployment

patrickhulce approved these changes Jul 8, 2019

View reviewed changes

add comment

ba9bc7c

vercel bot deployed to staging July 8, 2019 22:40 View deployment

brendankenny reviewed Jul 8, 2019

View reviewed changes

Update lighthouse-core/test/audits/dobetterweb/no-vulnerable-librarie…

e01212b

…s-test.js Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

googlebot added cla: no and removed cla: yes labels Jul 9, 2019

vercel bot deployed to staging July 9, 2019 00:05 View deployment

connorjclark added cla: yes and removed cla: no labels Jul 9, 2019

semver.Range

6ec1ae1

merge

441d4f8

googlebot added cla: no and removed cla: yes labels Jul 9, 2019

vercel bot temporarily deployed to staging July 9, 2019 01:04 Inactive

comments

55bf357

vercel bot deployed to staging July 9, 2019 01:05 View deployment

connorjclark added cla: yes and removed cla: no labels Jul 9, 2019

brendankenny reviewed Jul 10, 2019

View reviewed changes

update

4653ba1

googlebot added cla: no and removed cla: yes labels Jul 10, 2019

vercel bot deployed to staging July 10, 2019 21:27 View deployment

connorjclark added cla: yes and removed cla: no labels Jul 10, 2019

brendankenny approved these changes Jul 20, 2019

View reviewed changes

brendankenny merged commit f4572a4 into master Jul 20, 2019

brendankenny deleted the snyk-glob branch July 20, 2019 00:57

paulirish pushed a commit that referenced this pull request Nov 6, 2019

tests: assert vulnerable-library ranges have upper bounds (#9308)

2e70b99

connorjclark mentioned this pull request Dec 28, 2019

Don't allow * snyk vulns #8748

Closed

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

connorjclark mentioned this pull request Jun 29, 2022

Snyk vulnerability range for unfixed vulnerabilities #14168

Closed

GoogleChrome deleted a comment from kimkumsook-g Jul 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests(snyk): assert upper bounds #9308

tests(snyk): assert upper bounds #9308

tests(snyk): assert upper bounds #9308

tests(snyk): assert upper bounds #9308

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment