-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changing package.json to use concat-stream "^2.0.0" and tap "^12.5.3"… #1899
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the breaking change here?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the breaking changes here?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing. I have some fresh commits pending the PR so this is all backwards compatible.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.