http: don't inherit from Object.prototype

[mscdex]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• the commit message follows commit guidelines

Affected core subsystem(s)

• http

Description of change

This commit safely allows header names that are named the same as properties that are ordinarily inherited from Object.prototype such as __proto__.

[mscdex]

CI: https://ci.nodejs.org/job/node-test-pull-request/2205/

I also took the liberty of putting the prototype-less constructor in internal/util this time for better reusability. We could reuse this constructor in #6055 and #6092 and anywhere else where having this kind of object would be ideal.

[mscdex]

One other thing to note: the check introduced by 2a1ef97 was already preventing weird things from happening when setting header names that coincide with Object.prototype property names, but in some cases the header values would be ignored. With this PR, you can now actually see those special headers when they are received.

[mscdex]

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/188/

[jasnell]

This one could be fairly significant given how extensively the headers property is directly accessed. We'll want to make sure to get extensive review on this. @nodejs/http @nodejs/ctc

That said, the change LGTM

[Fishrock123]

Everyone accesses _headers.

[mscdex]

So it looks like there is a problem that is going to be a common theme for any of these instances where we replace {} with a prototype-less object: users of the should module will run into exceptions when trying to do something like req.headers.should.not.have.property('origin') or parsedQuerystring.should.have.property('asdf') because should only extends Object.prototype by default.

I'm not sure what to do about that...

[jasnell]

Yeah, that's the kind of breakage I was afraid of. I'm not sure there's
another solution tho unless we simply say certain types of values simply
aren't permitted... Which sucks.
On Apr 7, 2016 11:47 AM, "Brian White" notifications@github.com wrote:

So it looks like there is a problem that is going to be a common theme for
any of these instances where we replace {} with a prototype-less object:
users of the should module will run into exceptions when trying to do
something like req.headers.should.not.have.property('origin') or
parsedQuerystring.should.have.property('asdf') because should only extends
Object.prototype by default.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#6102 (comment)

[mscdex]

I suppose these libraries could monkey-patch Object.create() or something like:

var create = Object.create;
Object.create = function() {
  var obj = create.apply(null, arguments);
  if (obj.prototype == null)
    obj.prototype = Object.prototype;
  return obj;
};

Yeah it's hacky, but then again they're already mutating Object.prototype. I dunno...

[jasnell]

That's pretty ugly. I'm wondering if there's a chance anything can be done
on the v8 side to optimize the performance of setting the prototype after
creation so we don't get the perf regression. /cc @ofrobots
On Apr 7, 2016 11:55 AM, "Brian White" notifications@github.com wrote:

I suppose these libraries could monkey-patch Object.create() or something
like:

var create = Object.create;Object.create = function() {
var obj = create.apply(null, arguments);
if (obj.prototype == null)
obj.prototype = Object.prototype;
return obj;
};

Yeah it's hacky, but then again they're already mutating Object.prototype.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#6102 (comment)

[Fishrock123]

Idea: we expose a base null object and let people set onto that for these, then we inherit from it.

[mscdex]

@Fishrock123 So in this case you're saying expose StorageObject and modules like should would call Object.setPrototypeOf(StorageObject, Object.prototype) during startup?

[Fishrock123]

I would actually expose a "BaseObject" directly (Which would be a Object.create(null)) in some way, and then set that as the prototype for these.

I'm not 100% that is a good idea, but it is what first comes to mind..

[mscdex]

@Fishrock123 Wouldn't exposing the prototype vs the constructor complicate things by having to lazily set the constructor's prototype?

[Fishrock123]

@mscdex Wouldn't it still have the same reference everwhere? I don't think there would actually need to be lazy loading or special hooks.

[mscdex]

@Fishrock123 Oh I thought you meant have the ability to replace the prototype entirely and not just mutate it.

[jasnell]

working through the options I think it's likely going to be ok to simply require that if someone wants to treat these return values as objects they'll need to set the prototype themselves. We could make it slightly easier by providing a utility function but doing so isn't critical. It is a semver-major change, after all.

const headers = Object.setPrototypeOf(req.headers, Object.prototype);

or, using a utility method:

const headers = util.asObject(req.headers);

[evanlucas]

If this breaks headers.hasOwnProperty() I'm -1 on it.

[addaleax]

@evanlucas Thing is, that might also get broken by receiving a __proto__: null header in the HTTP response…

[evanlucas]

@addaleax That is a good point

[evanlucas]

@addaleax but that will be null as a string, not the null value.

[addaleax]

@evanlucas Hm yeah, you’re right… this still feels like the right kind of thing to do. At least for HTTP, where incoming headers are always converted to lower-case, maybe one could cherry-pick hasOwnProperty and possibly toString from Object.prototype and leave it at that?

[jasnell]

Hmm... that's certainly possible. I would just be a bit worried about user expectations... that is, if the thing has hasOwnProperty and toString from Object, then the user may expect it to be a full regular Object. We can add documentation warning them away from that but it's worth considering. What do you think @mscdex ?

[mscdex]

I think I tend to agree that cherry picking methods like that could be misleading, so it should be all or nothing IMHO.

[mscdex]

Do any other @nodejs/collaborators have any suggestions/opinions/comments/etc. about this? Does @jasnell's suggestion about simply requiring end users to call setPrototypeOf() on these particular objects seem ok? I have no problem with that.

/cc @btd too as he seems to be the maintainer of should and may have some input?

[jasnell]

We should get some feedback from folks like @dougwilson also

[MylesBorins]

@mscdex is this something we still want to explore?

[jasnell]

This is still something that ultimately should happen but there's still quite a bit of uncertainty around what would actually break. If we're going to do it, then I'm thinking we should be printing deprecation warnings whenever any Object.prototype methods or properties are accessed on this object in Node.js 8.0.0 and actually do switch in Node.js 9.0.0

[mscdex]

@thealphanerd I've always been for it, hence the PR, but the issue for http has always been breakage. Apparently more people are affected by that than the same changes we made to querystring.parse().

[fhinkel]

ping @mscdex

[mscdex]

@fhinkel This will be stalled until either the breakage is no longer a concern or the community is weaned off of the likes of res.headers.hasOwnProperty(), etc. in their code.

[richardlau]

Sorry, accidental close.

[ljharb]

@mscdex given that the breakage should always be a concern, and 15+ years of it being a bad practice to chain .hasOwnProperty off of objects hasn't weaned people off of it sufficiently, that suggests this will never make progress unless it's decided to go ahead and suffer through the breakage in v8 or v9 (eg).

[mscdex]

@ljharb That was just one example though. As mentioned earlier in this PR, another example is users adding to Object.prototype, which was actually being done by some popular modules on npm (e.g. should). So with the changes in this PR, some tests would break, such as res.headers.should.have.property(...), and CTC would also need to agree to allowing breakage of those such cases as well.

[ljharb]

Gotcha. I consider "should" patterns deprecated in favor of "expect" patterns, but just like normal chaining that's still going to be around forever. Has the CTC been able to discuss this yet and come to a conclusion? If not, could it go on the agenda?

[refack]

Add note about type in docs

[refack]

If this ever matures is should include notes in the docs similier to e1cabf6#diff-69e1ac0b5bfc06e74f2c1ab7b062c6af

[Trott]

@mscdex Should this remain open? Does it make sense to at least rebase so we can CITGM it again?

[jasnell]

Going to close this due to lack of forward progress. @mscdex ... when you're ready to revisit this, feel free to reopen or open a new PR