nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 227 forks source link

v2.x - do not mutate core-util-is module #423

Closed kumavis closed 4 years ago

kumavis commented 4 years ago

report

v2.x modifes the exports of core-util-is

v2.x is still used by browserify's via stream-browserify

The replacements inserted by the build process include some workaround for util and inherits. This seems to be an unintentional modification of the core-util-is.

https://github.com/nodejs/readable-stream/blob/b3cf9b1f46eaa1865930ae03b96d7a4a570746f0/lib/_stream_readable.js#L66-L69

fix

The fix uses Object.create(...) to create a new object with the core-util-is module.exports set as its prototype. This means all the content of the original module.exports is exposed but the object can be decorated without mutating core-util-is.

I originally used Object.assign({}, ...) but Object.create has even wider support.

platform support details

context:

I'm working on a set of tools to help defend against software supplychain attacks. One of the imposed limitations is that you cannot modify the module.exports of a module from another package. readable-stream@2.x is one of the only packages I've found that breaks this limitation. readable-stream@3.x has such issue.

mcollina commented 4 years ago

v2.x still support Node.js 0.8, and I think Object.create() is not present there. Can you address that?

Overall I would concentrate your efforts in getting stream-browserify updated to v3.x as v2.x is ancient.

kumavis commented 4 years ago

test errors seem unrelated

mcollina commented 4 years ago

Does this apply to v3 as well?

kumavis commented 4 years ago

@mcollina thanks for the quick response!

did a quick test: Object.create does work in node v0.8 Object.assign is not present in node v0.8

readable-stream on  v2-no-util-mut [?] is 📦 v2.3.6 via ⬢ v0.8.28 took 18s 
⇡0% ➜ node
> process.version
'v0.8.28'
> Object.create
[Function: create]
> x={a:1};y=Object.create(x)
{}
> y.b=2
2
> y.b
2
> y.a
1
> x
{ a: 1 }
> Object.assign
undefined
kumavis commented 4 years ago

v3.x does not mutate core-util-is :+1:

https://github.com/nodejs/readable-stream/blob/edd8c2d87c0f43f4f1d78407ac972fc193e45b40/lib/_stream_readable.js#L92

kumavis commented 4 years ago

Overall I would concentrate your efforts in getting stream-browserify updated to v3.x as v2.x is ancient.

I agree, though I am not a maintainer on browserify/* and v3 is a breaking change. Trying to get the minimal fix through :bowing_man:

seems stream-browserify CI expects 0.8 support

here is a relevant browserify land stuff PR https://github.com/browserify/stream-browserify/pull/18 issue https://github.com/browserify/stream-browserify/issues/17 issue https://github.com/browserify/browserify/issues/1880

kumavis commented 4 years ago

@mcollina sorry to bring this ancient maintenance task upon you but it would unblock me on my quest to improve the security of open source projects. browserify's technical debt should not be your burden to bear. I will attempt to push through a breaking change on their end for streams@3

I humbly ask for your time in landing this 2.x patch

kumavis commented 4 years ago

@mcollina seems like the CI tries to install npm v6 which is not supported by node v4

kumavis commented 4 years ago

@mcollina Resolved the CI issues that were already present in the v2.x branch. modified the CI script to only update the npm version for those flagged legacy. otherwise default npm is used.

ljharb commented 4 years ago

It would be great to land this, and make the transition from v2 to v3 easier for people by reducing the delta between them.

mcollina commented 4 years ago

@ljharb from an API perspective, readable-stream v2 is a cut of the stream module from Node 8 (there has been no semver-major changes from Node 4 to 8). readable-stream v3 is a cut from Node 10. readable-stream v4 will be a cut from 12, or possibly 14. This module has very little control on what it can ship, minus the minimal Node.js and browser version.

Given the massive adoption of new ES5 and ES6 features in Node core, it has been deemed too hard for the current maintainers to ship support for IE < 11 and old nodes in v3: making the test harness “pass” with polyfills of new JS features was gargantuan with very little benefit. We are of course happy to take PRs that improve that situation.

(I’m currently on vacation, I’ll land this as soon as I can).

mcollina commented 4 years ago

released as v2.3.7.