Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Changing package.json to use concat-stream "^2.0.0" and tap "^12.5.3"… #1899

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Changing package.json to use concat-stream "^2.0.0" and tap "^12.5.3"… #1899

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c278d73
Changing package.json to use concat-stream "^2.0.0" and tap "^12.5.3"…
Mar 1, 2019
d2b15ca
changing concat-stream back to 1.6.0; making test code back-compatibl…
Mar 1, 2019
9096815
ticking down to a recent version of tap that doesn't use esm
Mar 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

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?

Copy link
Author

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

"console-browserify": "^1.1.0",
"constants-browserify": "~1.0.0",
"crypto-browserify": "^3.0.0",
Expand Down Expand Up @@ -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",
Copy link
Member

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?

Copy link
Author

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.

Copy link
Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

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

"temp": "^0.8.1",
"through": "^2.3.4"
},
Expand Down
4 changes: 2 additions & 2 deletions test/bare.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ test('bare', function (t) {
}
});
vm.runInNewContext(body, {
Buffer: Buffer,
Buffer: function (s) { return Buffer.from(s) },
Copy link
Member

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.

Copy link
Author

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.

Copy link
Author

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.

Copy link
Member

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!

console: {
log: function (msg) {
t.ok(Buffer.isBuffer(msg));
Expand Down Expand Up @@ -57,7 +57,7 @@ test('bare api', function (t) {
}
});
vm.runInNewContext(body, {
Buffer: Buffer,
Buffer: function (s) { return Buffer.from(s) },
console: {
log: function (msg) {
t.ok(Buffer.isBuffer(msg));
Expand Down
2 changes: 1 addition & 1 deletion test/bare_shebang.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ test('bare shebang', function (t) {
}
});
vm.runInNewContext(body, {
Buffer: Buffer,
Buffer: function (s) { return Buffer.from(s) },
console: {
log: function (msg) {
t.ok(Buffer.isBuffer(msg));
Expand Down
34 changes: 17 additions & 17 deletions test/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ test('utf8 buffer to base64', function (t) {
var c = context();
vm.runInNewContext(src, c);
t.equal(
new c.require('buffer').Buffer("Ձאab", "utf8").toString("base64"),
new Buffer("Ձאab", "utf8").toString("base64")
new c.require('buffer').Buffer.from("Ձאab", "utf8").toString("base64"),
new Buffer.from("Ձאab", "utf8").toString("base64")
);
});
});
Expand All @@ -27,8 +27,8 @@ test('utf8 buffer to hex', function (t) {
var c = context();
vm.runInNewContext(src, c);
t.equal(
new c.require('buffer').Buffer("Ձאab", "utf8").toString("hex"),
new Buffer("Ձאab", "utf8").toString("hex")
new c.require('buffer').Buffer.from("Ձאab", "utf8").toString("hex"),
new Buffer.from("Ձאab", "utf8").toString("hex")
);
});
});
Expand All @@ -42,8 +42,8 @@ test('ascii buffer to base64', function (t) {
var c = context();
vm.runInNewContext(src, c);
t.equal(
new c.require('buffer').Buffer("123456!@#$%^", "ascii").toString("base64"),
new Buffer("123456!@#$%^", "ascii").toString("base64")
new c.require('buffer').Buffer.from("123456!@#$%^", "ascii").toString("base64"),
new Buffer.from("123456!@#$%^", "ascii").toString("base64")
);
});
});
Expand All @@ -56,8 +56,8 @@ test('ascii buffer to hex', function (t) {
var c = context();
vm.runInNewContext(src, c);
t.equal(
new c.require('buffer').Buffer("123456!@#$%^", "ascii").toString("hex"),
new Buffer("123456!@#$%^", "ascii").toString("hex")
new c.require('buffer').Buffer.from("123456!@#$%^", "ascii").toString("hex"),
new Buffer.from("123456!@#$%^", "ascii").toString("hex")
);
});
});
Expand All @@ -70,8 +70,8 @@ test('base64 buffer to utf8', function (t) {
var c = context();
vm.runInNewContext(src, c);
t.equal(
new c.require('buffer').Buffer("1YHXkGFi", "base64").toString("utf8"),
new Buffer("1YHXkGFi", "base64").toString("utf8")
new c.require('buffer').Buffer.from("1YHXkGFi", "base64").toString("utf8"),
new Buffer.from("1YHXkGFi", "base64").toString("utf8")
);
});
});
Expand All @@ -85,8 +85,8 @@ test('hex buffer to utf8', function (t) {
vm.runInNewContext(src, c);
var B = c.require('buffer');
t.equal(
new B.Buffer("d581d7906162", "hex").toString("utf8"),
new Buffer("d581d7906162", "hex").toString("utf8")
new B.Buffer.from("d581d7906162", "hex").toString("utf8"),
new Buffer.from("d581d7906162", "hex").toString("utf8")
);
});
});
Expand All @@ -99,8 +99,8 @@ test('base64 buffer to ascii', function (t) {
var c = context();
vm.runInNewContext(src, c);
t.equal(
new c.require('buffer').Buffer("MTIzNDU2IUAjJCVe", "base64").toString("ascii"),
new Buffer("MTIzNDU2IUAjJCVe", "base64").toString("ascii")
new c.require('buffer').Buffer.from("MTIzNDU2IUAjJCVe", "base64").toString("ascii"),
new Buffer.from("MTIzNDU2IUAjJCVe", "base64").toString("ascii")
);
});
});
Expand All @@ -113,8 +113,8 @@ test('hex buffer to ascii', function (t) {
var c = context();
vm.runInNewContext(src, c);
t.equal(
new c.require('buffer').Buffer("31323334353621402324255e", "hex").toString("ascii"),
new Buffer("31323334353621402324255e", "hex").toString("ascii")
new c.require('buffer').Buffer.from("31323334353621402324255e", "hex").toString("ascii"),
new Buffer.from("31323334353621402324255e", "hex").toString("ascii")
);
});
});
Expand All @@ -126,7 +126,7 @@ test('indexing a buffer', function (t) {
b.bundle(function (err, src) {
var c = context();
vm.runInNewContext(src, c);
var buf = c.require('buffer').Buffer('abc');
var buf = c.require('buffer').Buffer.from('abc');
t.equal(buf[0], 97);
t.equal(buf[1], 98);
t.equal(buf[2], 99);
Expand Down