Do not leak Object.prototype when checking for core modules

[nicolo-ribaudo]

Context:

I'm trying to use Yarn 2 (with PnP) in Babel.
Babel depends on browserify, which depends on browser-resolve which depends on an older version of resolve without PnP support (1.1.7).

I tried to upgrade resolve in browser-resolve, but a test started failing because it checked for toString resolution.

Since this worked in resolve 1.1.7, was the behavior changed on purpose?

[ljharb]

I'm not sure why toString resolution would have changed; https://github.com/browserify/resolve/blob/master/lib/core.js#L49 should already prevent any prototype leaks - meaning that this change should not be needed to make the test here pass.

[nicolo-ribaudo]

meaning that this change should not be needed to make the test here pass.

The test I added in this PR fails without the patch. It's because resolve uses core[x] to check for builtins, instead of only checking own properties.

https://github.com/browserify/resolve/blob/master/lib/core.js#L49 isn't enough, because Object.prototype properties are already present in core's prototype chain even before running that loop.

[ljharb]

Gotcha. In that case, can we instead fix

resolve/index.js

Line 4 in cfd0bb8

async.isCore = function isCore(x) { return core[x]; };

and https://github.com/browserify/resolve/blob/master/lib/sync.js#L71 and https://github.com/browserify/resolve/blob/master/lib/sync.js#L78? (also the sync ones should probably call into the same isCore function on the main export)

[nicolo-ribaudo]

Already done in e5b0494!

[ljharb]

adding is-core makes this technically semver-minor :-) will merge into both master and 1.x, and release shortly.

[nicolo-ribaudo]

Thanks!

Btw, upgrading this package in browser-resolve has also other problems (second point in browserify/browser-resolve#93 (comment)). If I find any other issue/regression in this package I'll probably open another PR 😛

[ljharb]

hm, these go back prettttty far :-) but thanks, would be appreciated.

[ljharb]

v1.13.0 released.

[ljharb]

unfortunately this introduced a bug into v1.13.0 (see #231), by changing this from a "truthy" check, to a "presence" check - and the "core" object relies on true/false to differentiate between a core module in some version of node, versus a core module in the current version of node. This was inadvertently fixed when I extracted this logic into https://npmjs.com/is-core-module.