-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Scan for vulnerable JS Libraries #2372
Changes from 21 commits
b6551af
59f4ee9
24b8461
86ac495
9ebc72b
ac9cc34
f0fd224
56cad31
26af245
5d63089
0c700b2
0a23ee2
c398a92
77c34f8
118417b
d7fdc16
a7038dc
26b59be
f27b75e
ce6bd00
3bd16ef
c36914f
01d30ef
b0ac61a
ea8be78
de78540
c17d5fc
f27ce59
e55ac62
168aa50
24f708d
cd17774
f24dc9d
eb53e66
3fb21f8
678b63b
eb81ef2
ae7947b
b44728c
09b61a2
ee1f494
4578cd7
0adf078
a080f7c
2fcff67
a85d697
1c1db60
21c0a2c
d57b81a
a8edaf5
c07833e
d1a3756
cf3d743
250dbea
90f40f9
507588d
abb3246
b3e182e
5e45f33
ab8ff76
c0569e5
88ff656
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | |||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,90 @@ | |||||||||||
/** | |||||||||||
* @license | |||||||||||
* Copyright 2017 Google Inc. All rights reserved. | |||||||||||
* | |||||||||||
* Licensed under the Apache License, Version 2.0 (the "License"); | |||||||||||
* you may not use this file except in compliance with the License. | |||||||||||
* You may obtain a copy of the License at | |||||||||||
* | |||||||||||
* http://www.apache.org/licenses/LICENSE-2.0 | |||||||||||
* | |||||||||||
* Unless required by applicable law or agreed to in writing, software | |||||||||||
* distributed under the License is distributed on an "AS IS" BASIS, | |||||||||||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |||||||||||
* See the License for the specific language governing permissions and | |||||||||||
* limitations under the License. | |||||||||||
*/ | |||||||||||
|
|||||||||||
/** | |||||||||||
* @fileoverview Audits a page to make sure there are no JS libraries with | |||||||||||
* known vulnerabilities being used. | |||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mention the db is provided by snyk.io and checked in locally as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GTG |
|||||||||||
*/ | |||||||||||
|
|||||||||||
'use strict'; | |||||||||||
|
|||||||||||
const Audit = require('../audit'); | |||||||||||
|
|||||||||||
class NoVulnerableLibrariesAudit extends Audit { | |||||||||||
|
|||||||||||
/** | |||||||||||
* @return {!AuditMeta} | |||||||||||
*/ | |||||||||||
static get meta() { | |||||||||||
return { | |||||||||||
category: 'Security', | |||||||||||
name: 'no-vulnerable-libraries', | |||||||||||
description: 'Avoids front-end JavaScript libraries' | |||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should also include failureDescription. Eg "Includes known vulnerabilities" I get the following error: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
|||||||||||
+ ' with known security vulnerabilities', | |||||||||||
helpText: 'Some third-party scripts may contain known security vulnerabilities ' + | |||||||||||
' that are easily identified and exploited by attackers.', | |||||||||||
requiredArtifacts: ['JSVulnerableLibraries'] | |||||||||||
}; | |||||||||||
} | |||||||||||
|
|||||||||||
/** | |||||||||||
* @param {!Artifacts} artifacts | |||||||||||
* @return {!AuditResult} | |||||||||||
*/ | |||||||||||
static audit(artifacts) { | |||||||||||
const libraries = artifacts.JSVulnerableLibraries.libraries; | |||||||||||
|
|||||||||||
const finalVulns = Object.assign(...libraries.filter(obj => { | |||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found it, fixed it, and added a test. :) |
|||||||||||
return obj.vulns; | |||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also the formatting made this a little hard to follow const finalVulns = Object.assign(
...libraries
.filter(obj => {
return obj.vulns;
})
.map(record => {
const libVulns = [];
for (const i in record.vulns) {
if (Object.hasOwnProperty.call(record.vulns, i)) {
libVulns.push(record.vulns[i]);
}
}
return libVulns;
})
); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So Prettier was new to me. Preettttty cool stuff. |
|||||||||||
}).map(record => { | |||||||||||
const libVulns = []; | |||||||||||
for (const i in record.vulns) { | |||||||||||
if (Object.hasOwnProperty.call(record.vulns, i)) { | |||||||||||
libVulns.push(record.vulns[i]); | |||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think markdown is supported here, so could you make the Snyk URLs clickable? Also, is it possible to see the vulnerability itself in the results? For example:
Here's how it appears now: I also reordered the table so the lib is first but I nitpick. https://techcrunch.com/ used for the audit. Full results: https://googlechrome.github.io/lighthouse/viewer/?gist=81ecfb452d3e13104d0816e16a1dde1e There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So as far as I can see, there's no (current) way to get markdown rendered in a table. @paulirish, if I'm right, I'm happy to add it, but not sure if you would prefer that happens in a separate PR for tidiness. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No generic markdown support but there's a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you ok with me doing that on this PR? Just didn't know if I should break it out separately for any reason. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be super useful elsewhere too and is fairly standalone so might be easier to review in isolation 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #3154 |
|||||||||||
} | |||||||||||
} | |||||||||||
return libVulns; | |||||||||||
})); | |||||||||||
|
|||||||||||
let displayValue = ''; | |||||||||||
if (finalVulns.length > 1) { | |||||||||||
displayValue = `${finalVulns.length} vulnerabilities detected.`; | |||||||||||
} else if (finalVulns.length === 1) { | |||||||||||
displayValue = `${finalVulns.length} vulnerability was detected.`; | |||||||||||
} | |||||||||||
|
|||||||||||
const headings = [ | |||||||||||
{key: 'url', itemType: 'text', text: 'Details'}, | |||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We think the output could be slightly different, though admittedly it does require some changes on the synk side, but I think it's worth it. Basically, instead of this: We get:
And clicking through to the jquery page for example, is showing the details for that library, and with the affected version's vulnerabilities highlighted... The most basic version being something like... Hows that sound? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this differs slightly from @rviscomi's suggestion in that he wanted to see the vulnerability type directly in the report, whereas this just displays a count. Just want to confirm we're ok with no type before I implement.... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I defer to Paul and the LH maintainers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As noted above (#2372 (comment)), I don't see a way to make the links clickable with markdown in a table. Otherwise the formatting of the table has been updated. |
|||||||||||
{key: 'library', itemType: 'text', text: 'Library'}, | |||||||||||
{key: 'severity', itemType: 'text', text: 'Severity'} | |||||||||||
]; | |||||||||||
const details = Audit.makeV2TableDetails(headings, finalVulns); | |||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We just removed the "V2":
|
|||||||||||
|
|||||||||||
return { | |||||||||||
rawValue: finalVulns.length === 0, | |||||||||||
displayValue, | |||||||||||
extendedInfo: { | |||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, I think. :) So the way I was using it, apparently it was just unnecessary as I was also passing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All set. See 26af245 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @paulirish anything that's useful is also being surfaced in It's time to nip bloat in the bud. #2276 |
|||||||||||
js_libs: libraries, | |||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GTG |
|||||||||||
vulnerabilities: finalVulns | |||||||||||
}, | |||||||||||
details, | |||||||||||
}; | |||||||||||
} | |||||||||||
|
|||||||||||
} | |||||||||||
|
|||||||||||
module.exports = NoVulnerableLibrariesAudit; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
/** | ||
* @license | ||
* Copyright 2017 Google Inc. All rights reserved. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same license header comment. |
||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
/** | ||
* @fileoverview Gathers a list of libraries and | ||
any known vulnerabilities they contain. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this fit on one line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If not, add leading |
||
*/ | ||
|
||
/* global window */ | ||
/* global d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests */ | ||
|
||
'use strict'; | ||
|
||
const Gatherer = require('../gatherer'); | ||
const fs = require('fs'); | ||
const semver = require('semver'); | ||
const libDetectorSource = fs.readFileSync( | ||
require.resolve('js-library-detector/library/libraries.js'), 'utf8'); | ||
// https://snyk.io/partners/api/v2/vulndb/clientside.json | ||
const snykDB = JSON.parse(fs.readFileSync( | ||
require.resolve('../../../../third-party/snyk-snapshot.json'), 'utf8')); | ||
|
||
/** | ||
* Obtains a list of an object contain any detected JS libraries | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Obtains list of detected vulnerable JS libraries and their versions. |
||
* and the versions they're using. | ||
* @return {!Object} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. think: |
||
*/ | ||
/* istanbul ignore next */ | ||
/* eslint-disable camelcase */ | ||
function detectLibraries() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method only grabs the libraries. It has no idea about whether they're vulnerable yet. That happens later on in the gatherer. |
||
const libraries = []; | ||
for (const i in d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests in a comment? Does this name change based on the version of libraries.js? Need some info :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests is an array. Can you use |
||
if (Object.hasOwnProperty.call(d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests, i)) { | ||
try { | ||
const result = d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests[i].test(window); | ||
if (result === false) continue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about instead, push if there's a result?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GTG |
||
libraries.push({ | ||
name: i, | ||
version: result.version, | ||
npmPkgName: d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests[i].npm | ||
}); | ||
} catch(e) {} | ||
} | ||
} | ||
return libraries; | ||
} | ||
/* eslint-enable camelcase */ | ||
|
||
class JSVulnerableLibraries extends Gatherer { | ||
/** | ||
* @param {!Object} options | ||
* @return {!Promise<!Array<!Object>>} | ||
*/ | ||
afterPass(options) { | ||
const expression = `(function () { | ||
${libDetectorSource}; | ||
return (${detectLibraries.toString()}()); | ||
})()`; | ||
|
||
return options.driver | ||
.evaluateAsync(expression) | ||
.then(libraries => { | ||
// add vulns to raw libraries results | ||
const vulns = []; | ||
for (const i in libraries) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GTG |
||
if (snykDB.npm[libraries[i].npmPkgName]) { | ||
const snykInfo = snykDB.npm[libraries[i].npmPkgName]; | ||
for (const j in snykInfo) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GTG |
||
if (semver.satisfies(libraries[i].version, snykInfo[j].semver.vulnerable[0])) { | ||
// valid vulnerability | ||
vulns.push({ | ||
severity: snykInfo[j].severity, | ||
library: libraries[i].name + '@' + libraries[i].version, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: `${libraries[i].name}@${libraries[i].version}`, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GTG |
||
url: 'https://snyk.io/vuln/' + snykInfo[j].id | ||
}); | ||
} | ||
} | ||
libraries[i].vulns = vulns; | ||
} | ||
} | ||
return {libraries}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need an object? Just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GTG |
||
}) | ||
.then(returnedValue => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need the last return. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GTG |
||
return returnedValue; | ||
}); | ||
} | ||
} | ||
|
||
module.exports = JSVulnerableLibraries; |
Large diffs are not rendered by default.
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 have short license headers now... copy one from one of the other files?
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.
All set.