Node: Let's make a plan for bad getters and toString() methods

[cjihrig]

Version: master
Platform: all
Subsystem: src

It seems like these types of issues get opened pretty frequently. Node accepts an object as input, and one of the properties on that object is a getter that throws an error. Similar problems exist with toString() and others. It's usually reported as a security issue that bypasses JavaScript validation (when we have some in place) and crashes in the binding layer.

I tried to fix one such issue and was told that instead of fixing ad hoc, we should come up with a plan. I agree with that idea. So, let's come up with a plan. Should we ignore the problem? Ensure that getters and other potential problematic methods are corrected in the JS layer? Something else?

[Trott]

/cc @tniessen

[addaleax]

I’m good with the approach that e.g. #12371 is taking – handle everything properly in the C++ layer, forwarding possible exceptions to JS if they are encountered.

[aqrln]

Applied the "security" label as it may be concerned by this issue in, at least, some cases; please remove it if it's not relevant.

[addaleax]

I think so far all collaborators have indicated in the discussions here that those issues can’t reasonably be considered security issues.

[DemiMarie]

One (rather extreme) option is to:

• quit using -fno-exceptions for Node's C++ code
• throw C++ exceptions when JS exceptions occur
• catch and ignore the exception at the V8 callback interface.

[bnoordhuis]

I personally consider it a non-issue. I'd be more amenable if it was something you hit by accident but so far it's all been people playing at security researcher.

[tniessen]

I agree with @addaleax: I don't see a problem fixing it ad hoc, this does not appear to be a design problem. None of the reported issues I came across pose any real-world danger.

[bnoordhuis]

@tniessen By the way, your first comment (the one you deleted) about gradually updating everything to MaybeLocal<Value> is on the ball. It's happening slowly but surely whenever we make updates.

[cjihrig]

Thanks for the feedback. I'll close the issue. Feel free to continue the discussion.