Issues regarding the use of __proto__

[julianjensen]

Any chance that the use of __proto__ can be changed? Looping through some objects created in express needs a check for __proto__ which is, at least, violates the principle of least astonishment. And then there's this:

Form MDN:

Warning: Changing the [[Prototype]] of an object is, by the nature of how modern JavaScript engines
optimize property accesses, a very slow operation, in every browser and JavaScript engine. The effects
on performance of altering inheritance are subtle and far-flung, and are not limited to simply the time
spent in obj.__proto__ = ... statement, but may extend to any code that has access to any object
whose [[Prototype]] has been altered. If you care about performance you should avoid setting
the [[Prototype]] of an object. Instead, create a new object with the desired [[Prototype]] using
Object.create().

I understand that using Object.create(), or outright removing __proto__, would probably be too much of a change, how about a compromise:

Instead of

someObject.__proto__ = whatever;

use

Object.defineProperty( someObject, '__proto__', {
    enumerable: false,
    writable: true, 
    configurable: true, 
    value: whatever
}

There are many ways to avoid using the deprecated and detrimental technique and it would be nice to shed its use, especially with a new version coming up. Anyway, just a suggestion. If there is any interest in making this change, I wouldn't mind doing a pull request on it.

Julian

[julianjensen]

It's the enumerable: false that makes a difference here. That prevents it from showing up in most cases when looping through properties. It's still there and acts in every respect just like the current property other than becoming invisible.

[dougwilson]

Can you provide the code in which you are seeing that property show up in a loop though the properties?

[julianjensen]

I temporarily added this to my code this morning at the very beginning where I log incoming requests (I removed the logging code), just to verify that it wasn't me that was putting it in there somehow (I never use __proto__):

        this.express.use( ( req, res, next ) => {
            if ( Object.prototype.hasOwnProperty.call( req, 'query' ) && Object.prototype.hasOwnProperty.call( req.query, '__proto__' ) )
                console.log( 'req.query has __proto__' );
            next();
        } );

And it shows that the query object on Request always has a visible __proto__ as an own key. This means that something like .hasOwnProperty(), which is the normal procedure to filter out unwanted keys, won't work in this case, for two reasons:

1. Unlike the real __proto__, which lives on the prototype and isn't enumerable, this __proto__ acutally is an own property.
2. If you try to check for unwanted properties with something like req.query.hasOwnProperty( '__proto__' ) it throws an exception because the query object was created with Object.create( null ); which means it doesn't inherit the standard Object.hasOwnProperty() function. This is why I use Object.prototype.hasOwnProperty.call( req.query, '__proto__' ) in my example above. I think that newcomers to express and JavaScript might be a bit befuddled at this point.

            if ( req.query )
            {
                for ( const propKey of Object.keys( req.query ) )
                {
                    console.log( 'propKey:', propKey );   // => '__proto__'
                    // The following line throws an exception:
                    // TypeError: req.query.hasOwnProperty is not a function
                    console.log( 'is own?', req.query.hasOwnProperty( propKey ) ); 
                }

                for ( const propKey of Object.keys( Object.create( null ) ) )
                {
                    console.log( 'obj.create null propKey:', propKey ); // Never gets here
                }

                for ( const propKey of Object.keys( Object.create( {} ) ) )
                {
                    console.log( 'obj.create literal propKey:', propKey ); // Never gets here
                }
            }

So, if you want to loop through the keys in query to see what query parameters are available, things get a little strange. :)

Hope this helps clarifying the issue.

Julian

[julianjensen]

And, I forgot to add this:

C:\Users\Julian\code\express [master ≡]> grep -rHn __proto__ *
lib/application.js:97:    this.request.__proto__ = parent.request;
lib/application.js:98:    this.response.__proto__ = parent.response;
lib/application.js:99:    this.engines.__proto__ = parent.engines;
lib/application.js:100:    this.settings.__proto__ = parent.settings;
lib/application.js:230:        req.__proto__ = orig.request;
lib/application.js:231:        res.__proto__ = orig.response;
lib/express.js:44:  app.request = { __proto__: req, app: app };
lib/express.js:45:  app.response = { __proto__: res, app: app };
lib/middleware/init.js:28:    req.__proto__ = app.request;
lib/middleware/init.js:29:    res.__proto__ = app.response;
lib/request.js:31:  __proto__: http.IncomingMessage.prototype
lib/response.js:41:  __proto__: http.ServerResponse.prototype
lib/router/index.js:50:  router.__proto__ = proto;
test/req.is.js:11:    __proto__: express.request
test/req.range.js:8:    , __proto__: express.request

That's from the current master branch. I honestly didn't check this on the 5.0 branch.

Julian

[dougwilson]

We never touch the proto property on req.query are you sure that does not return true for any object in JavaScript in general?

[dougwilson]

Basically I guess I'm not clear on what is ask really is, especially since you are referring to req.query, which comes from whatever the parsing modules are doing, so I'm not sure if a PR here could alter that behavior. Perhaps if you put together a PR that could help understand what the suggested fix is, as I'm still not clear on what exactly is being reported here.

[julianjensen]

Upon further investigation, it would appear that the query object does not have the __proto__ property anymore. It turns out that one of my servers had an older version of express running on it. So, at some point, the __proto__ field was removed from the query object in express, so false alarm on that topic. However, it remains that the relatively prevalent use of __proto__ throughout express should probably be addressed at some point but it becomes less important as the property on the query object was the only obvious problem for the average user that I knew of.

Sorry to have taken up your time. Should you wish to remove the __proto__ fields from express at some point, I would be happy to be of service.

Thanks for the prompt attention,
Julian

[dougwilson]

Gotcha. If the issue is now simply about using it as a general thing vs there actually being a real issue, there are multiple issues you can read for past discussions like #2613 and the issues/PRs it links to.

Basically you cannot replace it with Object.create, as we are altering objects we are creating and you cannot replace it with Object.defineProperty, because it will not trigger the setter on the previous property, which is the entire point of setting it. We could replace them with Object.setPeototypeOf when we only support Node.js versions that actually have that available, which is not on the roadmap.