denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
94.69k stars 5.26k forks source link

WebIDL symbols shouldn't be exposed to user code #11720

Open ghost opened 3 years ago

ghost commented 3 years ago

The webidl.brand symbol is a symbol on Deno's WebIDL API instances. Symbols are observable to any user script.

This allows anyone to simply take it, and bypass any checks, e.g.

// gets symbols and strings
// in browsers, this is an empty array, in Deno this is an array with a length of one
const webidl_brand = Reflect.ownKeys(performance)[0];

// not a host object, but functions as a subclass in Deno's implementation
const my_performance = {
  __proto__: Performance.prototype,
  [webidl_brand]: webidl_brand,
};

// should throw a TypeError
console.log(my_performance.now());

Should this be allowed?

A webidl.branded WeakSet would be much more secure, in that it would ensure encapsulation.

crowlKats commented 3 years ago

This applies for any other symbol that we use for internal slots as well, not just webidl brands

ghost commented 3 years ago

This applies for any other symbol that we use for internal slots as well, not just webidl brands

Absolutely, this was just an example of a particular symbol. But is this intended to be a "feature," or is it more of a limitation of the implementation?

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

kitsonk commented 2 years ago

Sorry I missed this before, but I tend to agree... I ran into a challenge where I created something that extended EventTarget and logging to the console leaked a whole load if "internal" stuff.

Feels like we should have an internal symbol filter feature on inspect. @lucacasonato thoughts?

ghost commented 2 years ago

Feels like we should have an internal symbol filter feature on inspect.

Now, I do fully agree: having the internal implementation values revealed in Console functions does look ugly, and is quite misleading, but I'm talking from a purely functional standpoint, i.e. just code operating on these values.

Do you think it's worth making the implementation not show the extra properties?