Changing package.json to use concat-stream "^2.0.0" and tap "^12.5.3"… #1899
Conversation
… to remove npm audit security warngings; updating tests to use Buffer.from() where appropriate to remove deprecation warnings in tests
package.json
Outdated
| @@ -30,7 +30,7 @@ | |||
| "browserify-zlib": "~0.2.0", | |||
| "buffer": "^5.0.2", | |||
| "cached-path-relative": "^1.0.0", | |||
| "concat-stream": "^1.6.0", | |||
| "concat-stream": "^2.0.0", | |||
There was a problem hiding this comment.
What is the breaking change here?
There was a problem hiding this comment.
Fair point; I have been cleansing a lot of npm code and this change probably came on the heels of another package I updated to 2.0.0. IMO it's no harm, no foul to keep it at 1.6.0; and it's no-harm, no foul to tick it up. I am resubmitting a PR without the concat-stream 2.0.0
package.json
Outdated
| @@ -83,7 +83,7 @@ | |||
| "make-generator-function": "^1.1.0", | |||
| "semver": "^5.5.0", | |||
| "seq": "0.3.5", | |||
| "tap": "^10.7.2", | |||
| "tap": "^12.5.3", | |||
There was a problem hiding this comment.
What are the breaking changes here?
There was a problem hiding this comment.
npm audit kicks off a notice of 15 vulnerabilities, see
infintesimal@hishost:~/bw$ npm audit --parseable
install handlebars high npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/755 tap>nyc>istanbul-reports>handlebars Y
install hoek moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/566 tap>coveralls>request>hawk>boom>hoek Y
install hoek moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/566 tap>coveralls>request>hawk>cryptiles>boom>hoek Y
install hoek moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/566 tap>coveralls>request>hawk>hoek Y
install hoek moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/566 tap>coveralls>request>hawk>sntp>hoek Y
install tunnel-agent moderate npm install --save-dev tap@12.5.3 Memory Exposure https://npmjs.com/advisories/598 tap>coveralls>request>tunnel-agent Y
install lodash moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/782 tap>nyc>istanbul-lib-instrument>babel-generator>babel-types>lodash Y
install lodash moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/782 tap>nyc>istanbul-lib-instrument>babel-generator>lodash Y
install lodash moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/782 tap>nyc>istanbul-lib-instrument>babel-template>babel-traverse>babel-types>lodash Y
install lodash moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/782 tap>nyc>istanbul-lib-instrument>babel-template>babel-traverse>lodash Y
install lodash moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/782 tap>nyc>istanbul-lib-instrument>babel-template>babel-types>lodash Y
install lodash moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/782 tap>nyc>istanbul-lib-instrument>babel-template>lodash Y
install lodash moderate npm install --save-dev tap@12.5.3 Prototype Pollution https://npmjs.com/advisories/782 tap>nyc>istanbul-lib-instrument>babel-traverse>babel-types>lodash Y
I am accountable to others for the "best-effort" basis on keeping packages secure to current standards. Fixing tap to 12.5.3 results in npm audit returning 0 known vulnerabilities.
There was a problem hiding this comment.
After tap@12.5.3
infintesimal@hishost:~/bw$ npm install --save-dev tap@12.5.3
+ tap@12.5.3
added 300 packages from 185 contributors and audited 580 packages in 10.058s
found 0 vulnerabilities
There was a problem hiding this comment.
I don’t think vulns matter in dev deps, generally. In this case, this prototype pollution attack only affects when deep merging untrusted code, which can’t happen here.
There was a problem hiding this comment.
The same goes for handlebars, which is only 'vulnerable' with untrusted templates. Browserify's dependencies only use trusted static templates.
Iirc, tunnel-agent is in the dependency tree but not used at all.
Still, fixing these warnings can be worthwhile, even if only because they're annoying to the developer. In this case though, we're specifically using older versions because the newer versions don't support all our target platforms. If there is a a way to upgrade without losing compatibility, I'd be all for it.
There was a problem hiding this comment.
Agreed. I understand the dev deps is not relevant in a prod bundle. My security peers want to mitigate the risk wherever possible. I am looking at fresh conflicts in esm and the older node platforms the team supports. I will see if I can work something out. Right now build is breaking on esm.js. I'll be glad to have the PR accepted when I can prove that I can get through the build for all platforms. Cheers. Steve
test/bare.js
Outdated
| @@ -28,7 +28,7 @@ test('bare', function (t) { | |||
| } | |||
| }); | |||
| vm.runInNewContext(body, { | |||
| Buffer: Buffer, | |||
| Buffer: function (s) { return Buffer.from(s) }, | |||
There was a problem hiding this comment.
Buffer.from does not exist in old Node versions which browserify still supports—for this change, we need to check if it exists:
var buf = Buffer.from ? Buffer.from(s) : Buffer(s)or use a module like safe-buffer that polyfills the from method.
There was a problem hiding this comment.
Fixing. I have some fresh commits pending the PR so this is all backwards compatible.
There was a problem hiding this comment.
Would you prefer I use safe-buffer to what I patched up in three of the test files (bare.js, bare_shebang.js and buffer.js)? I can do that too.
There was a problem hiding this comment.
safe-buffer might be easier because you can just use Buffer.from in all the callsites and it'll do the right thing. I don't really mind either way though!
… to remove npm audit security warngings; updating tests to use Buffer.from() where appropriate to remove deprecation warnings in tests
Fixes #1900.