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

Alternative "Delete the __proto__ and * keys" non-viable #4

Open
bmeck opened this issue Feb 7, 2023 · 5 comments
Open

Alternative "Delete the __proto__ and * keys" non-viable #4

bmeck opened this issue Feb 7, 2023 · 5 comments

Comments

@bmeck
Copy link
Member

bmeck commented Feb 7, 2023

Global disabling of .prototype and .constructor keys is unlikely to be viable. Node does have --disable-proto that it wished to turn on by default and even then the ecosystem has struggled to make it work (last run of our ecosystems tests still had it failing). I think deleting of __proto__ is possible but harder than framed given attempt to do so already.

Unfortunately due to some specifications like web components properties on these are intended to be accessed and would require major shifts to make them change not just in specifications but also in polyfills etc. See https://html.spec.whatwg.org/multipage/custom-elements.html#element-definition

@syg
Copy link
Collaborator

syg commented Feb 7, 2023

We agree disabling static property access like .prototype and .constructor is non-viable. We're only proposing to disable computed property access, i.e. obj[k] where k evaluates to "prototype" or "constructor" etc. Does your intuition extend to computed property access as well?

@bmeck
Copy link
Member Author

bmeck commented Feb 7, 2023

I wouldn't call it intuition given multiple CI attempts failing at this area. I think various tooling like closure compiler forcing computed property access to prevent mangling would make this unlikely in a different area if that is your question. I think any time .x and ["x"] would differ as a very new area for JS to break expectations as many tools in the past have exploited the 2 forms in order to do specific behaviors; though that isn't really why I find these particular alternatives unlikely to be viable.

@syg
Copy link
Collaborator

syg commented Feb 7, 2023

Ah okay. Whenever you get a chance, an instance of a CI failure would be interesting to look at.

@bmeck
Copy link
Member Author

bmeck commented Feb 7, 2023

@syg we don't keep them for multiple years but you can see some stuff at a glance from nodejs/node#39824 (comment)

@salcho
Copy link
Collaborator

salcho commented Mar 17, 2023

Hey Bradley, sorry for the late reply. I took a closer look at toolchains that produce code that is not directly compatible with this proposal and have updated the proposal to reflect what I've found. Please take a look at the updated content.

In general, it seems that the majority of public websites on the Internet would be compatible with this out of the box, but there is a non-negligible portion that would need to be refactored. That is a deal breaker for enabling secure mode by default, since it isn't backward compatible. The salient point here is that applications that are not compatible are more prone to being vulnerable, so by refactoring an application into compatibility, developers are reducing the places where computed properties have unintentional effects.

On the one hand, public crawl data indicates that many toolchains are compatible by default and on the other it points at the fact that there are well-lit paths to refactor incompatible toolchains into compatibility. For example, instead of producing const p = '__proto__'; obj[p]; the minimized code could produce const p = Symbol.instanceProto; obj[p] which has the same benefits in terms of code size and is compatible with secure mode. This can be worked around by either enacting changes in toolchains (high cost) or refactoring codebases to migrate away from string properties and into Symbol properties. This last option lends itself well for automated refactorings (find + replace). Since secure mode is proposed as an opt in feature, this seems like a reasonable compromise.

What seems more worrying to me is the point that external specifications have references to constructors embedded in them. I have been able to find 2 types of references: 1) where a spec asks 'what is the constructor of x' and 2) where a spec asks 'is this object a constructor'. I believe the second type will be incompatible with secure mode.

It would be very useful to hear feedback, especially on this last point, since I'm not sure that I understand all of its implications.

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

No branches or pull requests

3 participants