Scan for vulnerable JS Libraries

[tkadlec]

Fixes #1948

Uses https://github.com/johnmichel/Library-Detector-for-Chrome and Snyk to check a page for JS libraries with known vulnerabilities.

A few things that still need to be done for this to ship.

• Pull https://raw.githubusercontent.com/johnmichel/Library-Detector-for-Chrome/master/library/libraries.js into LH to be used
• Took a stab at mapping here: https://gist.github.com/tkadlec/83146d8505d7748e8944bc333c166ad3. This will also need to be auto-pulled in.
• Any stylistic changes/scoring changes that need to be made. Leaving that up to the LH team for priorities, but happy to chime in.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[patrickhulce]

this is huge @tkadlec thanks so much for putting this together!! 🎉 🔐

[patrickhulce]

want to make these separate files which we can pass into the detectLibraries function as arguments with JSON.stringify or something? the accessibility audit does something similar with axe to try to keep the gatherer logic a bit separate

[tkadlec]

Definitely. In the case of both, they're coming from GH/Gist repos so we'll need a way to pull them in semi-regularly to make sure the latest versions are included. Would something akin to HTTPArchive/legacy.httparchive.org@5a624cb#diff-4df11ed88be41bcc1851345666d6f7b1 be the best option?

[paulirish]

take a look at how we do the accessibility gatherer. we readFileSync the axe.min.js from the axe-core node module.

this way, versioning is pretty straightfoward as its just a node module dep bump.

[tkadlec]

Would love to do this, but Library Detector isn't an npm module at the moment. :(

Maybe we can sweet talk @johnmichel into publishing it?

[paulirish]

sgtm. filed johnmichel/Library-Detector-for-Chrome#98 to see if https://github.com/johnmichel/Library-Detector-for-Chrome/blob/master/library/libraries.js can be shipped on NPM

i can see a lot of activity on that file, so i'd prefer to have a nice versioning story for this first.

[patrickhulce]

we wont' really be able to see these log statements easily, how about pushing these messages onto an array that we return along with libraries?

[patrickhulce]

nit: can we construct the entire body as an object and JSON.stringify the whole thing instead of just dependencies?

[patrickhulce]

is there a rate limit on this token/who manages it? i don't know much about snyk 😄

[tkadlec]

This is a special one I setup just for Lighthouse, so no—no limit.

[rviscomi]

Does the token need to be kept secret? Can anyone just copy it can get unlimited snyk API calls?

[tkadlec]

The assumption was that given the nature of Lighthouse (where contributors will need to be able to clone and develop locally), a secret token wouldn't be very practical.

We can monitor this particular token for any non-Lighthouse usage on our end. So no...it won't be very useful to anyone else. :)

[ebidel]

WDYT think about contributing https://gist.github.com/tkadlec/83146d8505d7748e8944bc333c166ad3 upstream? I worried about maintaining new file like this without the help of the security community.

[ebidel]

remove "using any"

[ebidel]

This repeats the description. Is there more we can say? Also throw in a [Learn more]() link to somewhere relevant?

[tkadlec]

For the link, it looks like most (all?) other audits point to a page on Google Developers. Does it make sense to put something similar to https://developers.google.com/web/tools/lighthouse/audits/contrast-ratio together for this?

[ebidel]

Yep. @kaycebasques can work on that once we have the audit :)

[wardpeet]

I don't think it has to be a developers.google.com blog post. Maybe it could be a snyk page that has some information about this? As long as it's explaining why we do it.

FYI: i'm not against adding it to the audit references :)

[kaycebasques]

The typical pipeline is to link to some external doc while we put together an official doc on developers.google.com. The goal is to just provide some sort of further guidance for peeps that fail the test, so they're not like "wtf I'm failing this test and I have no idea what it means or how to fix it"

[ebidel]

comment looks out of date...?

[tkadlec]

Embarassing! :) Fixed.

[ebidel]

nit: just return the obj like so:

const finalVulns = vulns.map(record => ({
  severity: record.severity,
  library: record.name + '@' + record.version,
  url: 'https://snyk.io/vuln/' + record.id
}));

[tkadlec]

GTG

[ebidel]

nit:

rawValue: vulns.length === 0,
displayVrawValue,
extendedInfo: {...

[tkadlec]

GTG

[ebidel]

extendedInfo is legacy in LH 2.0. What do you think about just returning the new detail object. Our JSON results are getting out of control :)

[paulirish]

i wouldn't say it's "legacy", but its there only for folks consuming more details in the LHR, usually via the lighthouse module. not there for presentational purposes.

I think this sorta return would make sense:

extendedInfo: {
  value: {
    vulnerabilities: finalVulns
  }
}

[tkadlec]

Gotcha, I think. :)

So the way I was using it, apparently it was just unnecessary as I was also passing details. @rviscomi suggested including a full list of detected libs here as well, so I'm thinking:

extendedInfo: {
  value: {
    js_libs: libraries,
    vulnerabilities: finalVulns
  }
}

[tkadlec]

All set. See 26af245

[ebidel]

@paulirish anything that's useful is also being surfaced in details.items. If module consumers are pulling lower level details themselves, they're probably having a heck of time figuring out which of these to use/trust:

It's time to nip bloat in the bud. #2276

[tkadlec]

@ebidel More than happy to put that where ever it makes sense. LH? Library Detector?

[ebidel]

Library Detector?

Yea maybe? I'd imagine it may be useful to folks outside of Lighthouse.

[tkadlec]

Good call. Submitted a PR there. johnmichel/Library-Detector-for-Chrome#97

[brendankenny]

To properly use the code, if at all possible we should pull in the library as a node module, similar to how axe is done.

If not, it'll need to go into third_party/ along with its LICENSE file.

[paulirish]

nice. thanks for working on this. pretty cool to read through. :)

[paulirish]

i wouldn't say it's "legacy", but its there only for folks consuming more details in the LHR, usually via the lighthouse module. not there for presentational purposes.

I think this sorta return would make sense:

extendedInfo: {
  value: {
    vulnerabilities: finalVulns
  }
}

[paulirish]

take a look at how we do the accessibility gatherer. we readFileSync the axe.min.js from the axe-core node module.

this way, versioning is pretty straightfoward as its just a node module dep bump.

[brendankenny]

Two concerns about the this PR's approach (vs what @ebidel had sketched out in #1948):

• Since gatherers run serially, this puts an API endpoint in our critical path. While we block Lighthouse execution on requests in other gatherers, this will be the first one that's not the developer's fault, i.e. not the fault of a resource from the site itself.

• Currently our FAQ has

  "Lighthouse runs locally, auditing a page using a local version of the Chrome browser installed the machine. Report results are never processed or beaconed to a remote server."

  While this is limited to just sending npm packages and their version numbers, that wouldn't be true anymore after this change.

#1948 seemed to lean more toward just including the Snyk database. While that would likely be larger and it looks like the db is updated less often than the service, it would allow vulnerabilities to be tested entirely locally.

There wasn't a lot of discussion in #1948 about implementation approaches, so I'd be interested in discussing the tradeoffs so everyone knows what we're getting into (I'm sure all of this has come up before for some of you, but not all of us :)

[ebidel]

@brendankenny when I was working on integrating HTTPArchive data into the report, I came up with a system that users opt'd into for pulling live data vs. using cached data. For "live" mode, running LH would pull from a live endpoint and cache the data. Subsequent runs of LH would use the cache for 24hrs (also easy to increase). For "offline" mode, LH would use a checked in version of the data. We could rev that per release or something.

Something opt'in might be nice for users that want the most up to date information. But agreed, an opt in soln would be good. A caching layer at the very least.

[brendankenny]

what's the right way to move forward in that case? An offline solution with manual dependency updates for now, and then it sounds like @ebidel you've taken a look at a datasource update mechanism, so we could maybe write it as a general service that audits like this one could use?

Though it's worth noting the difference between checking via the API as is currently in the PR and

For "live" mode, running LH would pull from a live endpoint and cache the data

basically pulling data to locally check vs sending data to a service to check it remotely

[ebidel]

basically pulling data to locally check

+1 we should play it safe. Pull data, never send it.

[tkadlec]

Ok...just wanted to let the dust settle. :)

+1 we should play it safe. Pull data, never send it.

So do we want to ditch the API call? Just did that for simplicity. We could also provide a link to a JSON version of our DB (without all the unnecessary languages). That could be pulled in periodically and would be more up to date that the DB @brendankenny found.

That way, nothing is sent externally. The libraries are tested internally against the most recent copy of the JSON file and you report the vuln information discovered. Sound like a plan?

[ebidel]

@tkadlec sounds reasonable to bring in the data as a dep and cross reference it. https://github.com/snyk/vulnerabilitydb/tree/master/data/npm looks like it's updated ~monthly, so we could latch onto that timeline for our updates. It also looks like a bunch of individual json data files. Will we need a tool that aggregates them into a single file? For our binary size, I'd be curious how big that gets.

[tkadlec]

@ebidel Oh, just to clarify...I don't think you should pull from https://github.com/snyk/vulnerabilitydb/tree/master/data/npm. It updates slowly and brings unnecessary overhead.

What I'm proposing is a JSON file (hosted on S3) that is already flattened and excludes unnecessary languages/etc. We do this already for several partners we work with...so it's no problem to have one for Lighthouse either. That keeps it light and pruned, and updated each time we publish a new vuln.

You folks could decide how aggressive you want to be in pulling in a new version (looking at modified date, or on some regular time cadence).

[ebidel]

Oh, gotcha. So it would be "offline" in the sense that we'd cache your endpoint per LH release. That's the same approach I was taking for http archive integration. I setup an endpoint that hosts the latest snapshot: https://lighthouse-viewer.appspot.com/data.

@tkadlec If it's easy, is that something you can setup and we can experiment with? It would be nice to have some testing before committing to something long term. You know, support and stuff :)

[tkadlec]

@ebidel: Definitely! I can have it setup tomorrow.

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[paulirish]

@googlebot shh baby its ok

I made a few rote changes. Boring stuff.

[paulirish]

I think this is landable.

[brendankenny]

looking good!

this also needs a smoke/end-to-end test. Maybe we could take the do better web tester and adjust the CDN loaded version of jquery (already being used for an is-on-https check) so that it triggers a hit? The expected output would then go in dbw-expectations.js

[brendankenny]

can you add a more specific shape to these types?

[tkadlec]

GTG

[brendankenny]

same here

[tkadlec]

GTG

[brendankenny]

do you need empty extendedInfo here? Can just leave off

[tkadlec]

Done

[brendankenny]

nit: can get rid of this map block by assigning numericSeverity up in getVulns()

[tkadlec]

Done.

[brendankenny]

maybe call this highestSeverity since it's returning the highest severity level, not the most severe vulnerability? (at least that's how I interpreted mostSevere() :)

[tkadlec]

Good call. Done.

[brendankenny]

add new line at end

[tkadlec]

Done

[brendankenny]

can we nest this file so it's in third-party/snyk/snapshot.json (or whatever filename)?

[tkadlec]

Sure. Done. :)

[brendankenny]

also, would you mind maybe rebasing so we get the bundlesize fix picked up?

[paulirish]

LGTM from me.

When brendan is good, we'll merge.

because.

[tkadlec]

Added an end to end test as well. Think that's what @brendankenny had in mind.

[brendankenny]

added a minor change to a few of the jsdoc types and moved the json file from being reloaded every time the getter is called to only once

[brendankenny]

and the end to end test is great :)

[brendankenny]

☣️ 🔬 🚫

LGTM! Thank you for sticking with this verrrrry long process :)

[paulirish]

OMG. We merged. 🤗

[patrickhulce]

woohoo! very well done @tkadlec many thanks!! 🎉 🍰 💯

[igrigorik]

Yay! Kudos for the awesome work, everyone. Can't wait to see this live.

[tkadlec]

Yay! Looking forward to seeing this out. :) Thanks for the patience!