util: protect against monkeypatched Object prototype for inspect()

[Trott]

Prevent effects of monkeypatching (for example) Object.keys() when calling util.inspect().

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2521/pipeline

[Trott]

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

[BridgeAR]

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.

[Trott]

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)

Do you mean prevent Object.keys() monkey-patching, or do you mean prevent this internal module from using a monkey-patchable Object.keys() (by, say, caching/saving the original value of Object.keys() before a user is able to get in and change it)?

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.

[Trott]

@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 Object.keys()so that it isn't monkey-patchable, the code might look like this (with necessary line-wrapping added):

try {
      keys = Object.keys(value);
    } catch (err) {
      assert(isNativeError(err) &&  err.name === 'ReferenceError' && isModuleNamespaceObject(value));
      keys = Object.getOwnPropertyNames(value);
    }      

[BridgeAR]

@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.

[Trott]

@BridgeAR OK, I've updated this to avoid using a monkey-patched Object in inspect.js and modified the test to confirm that nothing breaks when Object is monkey-patched. And I've converted the guard code to use the new internal micro-assert(). PTAL!

@addaleax This is a significant enough change from the one you approved that you may wish to re-review?

[Trott]

Defensively adding semver-major in case this somehow breaks APMs or something. (Seems unlikely, but weirder stuff has happened. Happy to remove semver-major if that's the consensus.)

/ping @nodejs/tsc (for reviews, because: semver-major)

[Trott]

@bmeck @ljharb

[Trott]

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

[ljharb]

nevermind

To be robust, there can't ever be any runtime property lookups on mutable objects.

[addaleax]

@ljharb These are not the original builtins, they are frozen copies that are not accessible to userland.

[ljharb]

@addaleax ahh that's the context i'm missing, thanks.

[Trott]

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20678/ ✅

[Trott]

Landed in 1847696

[Trott]

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/