util: protect against monkeypatched Object prototype for inspect() #25953
Conversation
There was a problem hiding this comment.
I don't think that this is actually a good test. We create an artificial error that can only happen due to Object.keys being monkey patchable.
If we prevent that (which should IMHO be done at some point) this would not be possible anymore. So instead, I would rather make sure we prevent that. Afterwards it will AFAIK not be possible to trigger that code line at all but that should be fine in this case since it's just a safe guard against spec changes.
Do you mean prevent Assuming the latter, I think this is a good test to have until we actually do that, and then it can be removed. What this test does in its current form is test that the guard-against-spec-changes (and also guard-against-V8-bugs) is working as expected. |
|
@BridgeAR Ooh, actually perhaps a thing we could both be happy with is changing the guard code to an assertion. So once we've cached try {
keys = Object.keys(value);
} catch (err) {
assert(isNativeError(err) && err.name === 'ReferenceError' && isModuleNamespaceObject(value));
keys = Object.getOwnPropertyNames(value);
} |
|
@Trott I am fine switching to the assertion as you suggested. We could even log the actual error (just passing through the error as message). That could also be done without preventing Object.keys from being monkey patchable here. That way the line will be properly covered without actually changing the behavior. |
Prevent affects of monkeypatching (for example) Object.keys() when calling util.inspect().
3d11b79 to
7ba6848
Compare
|
@BridgeAR OK, I've updated this to avoid using a monkey-patched @addaleax This is a significant enough change from the one you approved that you may wish to re-review? |
|
Defensively adding /ping @nodejs/tsc (for reviews, because: |
|
@ljharb These are not the original builtins, they are frozen copies that are not accessible to userland. |
|
@addaleax ahh that's the context i'm missing, thanks. |
|
Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20678/ ✅ |
|
Landed in 1847696 |
Prevent affects of monkeypatching (for example) Object.keys() when calling util.inspect(). PR-URL: nodejs#25953 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Ugh, forgot a CITGM run. Sorry, everyone. Here's one after-the-fact (run against master since this landed): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1734/ |
Prevent effects of monkeypatching (for example) Object.keys() when calling util.inspect().
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes