Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Object.setPrototypeOf as __proto__ could potentially be disabled #387

Merged
merged 1 commit into from Apr 2, 2020

Conversation

bmeck
Copy link
Contributor

@bmeck bmeck commented Mar 10, 2020

Due to CVEs and accidents, Node is thinking of allowing disabling Object.prototype.__proto__. This PR achieves the same functionality using Object.setPrototypeOf which does not suffer the same issues and removes a lint error.

See: nodejs/node#31951

@ljharb
Copy link

@ljharb ljharb commented Mar 10, 2020

Note that this is a breaking change; as there are many old browsers that support __proto__ but not Object.setPrototypeOf, including all IE.

It would be better to use something like the setprototypeof package, so it uses the newest technique available (which would also still work in the unlikely event of node removing dunder proto)

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Mar 10, 2020

@ljharb Chalk is not a browser package.

@ljharb
Copy link

@ljharb ljharb commented Mar 10, 2020

lol fair, but s/browser/engine. node 0.10 and below don't have Object.setPrototypeOf, which still makes this PR a breaking change - using the setprototypeof package or similar would avoid that.

@bmeck
Copy link
Contributor Author

@bmeck bmeck commented Mar 10, 2020

If it is considered breaking, that seems fine to neutral from my perspective, but I would prefer not to use the package which would add a dependency and have questionable gains. The last release in question on that node release line is from 2016 and has multiple known security issues.

@ljharb
Copy link

@ljharb ljharb commented Mar 10, 2020

The gain is that by not being a breaking change, the fix you want will filter out to the maximum number of users quickly. You could certainly after that publish a major version that drops the dependency if you wanted.

@bmeck
Copy link
Contributor Author

@bmeck bmeck commented Mar 10, 2020

@ljharb I'd personally prefer the major over using the dependency given supply chain concerns and lack of clear reasons to continue support for engines with known major issues.

@ljharb
Copy link

@ljharb ljharb commented Mar 10, 2020

I don’t understand “supply chain concerns”; chalk itself is a dependency too, and this PR works by the same mechanism.

Whether to continue support for an engine or not is a separate decision - capriciously dropping support just to avoid adding a dep seems hostile to me, and in particular, antithetical to your goals of getting this PR out into the ecosystem.

@bmeck
Copy link
Contributor Author

@bmeck bmeck commented Mar 10, 2020

@ljharb My viewpoint is unchanged. I personally would not consider breakage in platforms that are out of the support matrix and their own support matrix as breaking changes. I do not think adding a dependency is a trivial matter but you might. I do not see any hostility, but if you wish to view my opinions as such you are free to frame them as you please. The goal of this PR is to prepare for situations where __proto__ is potentially unavailable, which it does.

@ljharb
Copy link

@ljharb ljharb commented Mar 10, 2020

It also makes chalk brittle if someone deletes Object.setPrototypeOf, but that’s easily fixable, at least.

@bmeck
Copy link
Contributor Author

@bmeck bmeck commented Mar 10, 2020

Adding a dependency also adds some form of brittle-ness (registry going down / removal from registry / removal from disk / etc.). I don't see any major point to add to this discussion really.

@jridgewell
Copy link

@jridgewell jridgewell commented Mar 10, 2020

node 0.10 and below don't have Object.setPrototypeOf, which still makes this PR a breaking change

Chalk supports node 8 and above:

chalk/package.json

Lines 9 to 11 in 797461e

"engines": {
"node": ">=8"
},

@bmeck
Copy link
Contributor Author

@bmeck bmeck commented Mar 10, 2020

It does look like a variety of other things being used are not available in node 0.10.x like

https://github.com/chalk/chalk/blob/master/source/templates.js#L29

@ljharb
Copy link

@ljharb ljharb commented Mar 10, 2020

@jridgewell ah, alright then - fair enough.

@ljharb
Copy link

@ljharb ljharb commented Mar 10, 2020

In that case, Object.setPrototypeOf should still be cached at module level rather than freshly looked up off of Object, but I withdraw my other comments.

@bmeck
Copy link
Contributor Author

@bmeck bmeck commented Mar 10, 2020

I'll leave that to the maintainers, but existing usage in

chalk/source/index.js

Lines 41 to 42 in 797461e

Object.setPrototypeOf(chalk, Chalk.prototype);
Object.setPrototypeOf(chalk.template, chalk);
does not cache the value, nor do any of the other primordials/methods get cached. I think such a change for robustness could be discussed in a different PR.

@ljharb
Copy link

@ljharb ljharb commented Mar 10, 2020

Fair enough as well.

@bmeck
Copy link
Contributor Author

@bmeck bmeck commented Mar 20, 2020

node has landed the CLI flag to disable __proto__ nodejs/node#32279 and deno now always disables it denoland/deno#4341

@devsnek
Copy link

@devsnek devsnek commented Mar 23, 2020

I don't understand how Object.xyzPrototypeOf is more of a deletion hazard than Object.prototype.__proto__ (in fact i think __proto__ is technically more of a deletion hazard because people keep deleting it, which is why this pr exists in the first place).

@Qix-
Copy link
Member

@Qix- Qix- commented Mar 25, 2020

Just chiming in that there are a number of setPrototypeOf-based attacks floating around out there. Caching the function may not be the worst idea.

@ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Mar 25, 2020

Don’t forget that Object.setPrototypeOf can be polyfilled in ES5 (and some ES3) environments that implement __proto__ as an accessor property on Object.prototype, in which case, one can do:

Object.defineProperty(Object, "setPrototypeOf", {
	configurable: true,
	writable: true,
	value: Function.prototype.call.bind(Object.getOwnPropertyDescriptor("__proto__").set)
});

@bmeck
Copy link
Contributor Author

@bmeck bmeck commented Mar 31, 2020

Node 13.12.0 now ships with the flag to remove Object.prototype.__proto__

@sindresorhus sindresorhus changed the title use Object.setPrototypeOf as __proto__ could potentially be disabled Use Object.setPrototypeOf as __proto__ could potentially be disabled Apr 2, 2020
@sindresorhus sindresorhus merged commit 63469d3 into chalk:master Apr 2, 2020
1 check passed
@MatthiasKunnen
Copy link

@MatthiasKunnen MatthiasKunnen commented Jul 21, 2020

If anyone wants to use --disable-proto but a dependency with an old version of chalk is holding you back, with Yarn's selective dependency resolutions you can force chalk to version 4.

Only recommended for forcing version 3 to 4. Lower versions might be a problem due to breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants