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

Prevent polluting prototypes with enumerable props #879

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JGreenlee
Copy link

@JGreenlee JGreenlee commented Aug 23, 2024

Fixes #816

Assigning functions directly to the prototypes of Number, Array, String, etc., causes them to be enumerable, meaning they can show up in "for .. in .." loops and cause unexpected behavior!

Instead we can use Object.defineProperty (for which there is already a helper called __setproperty__). With this method, i) new props are non-enumerable by default, and ii) we can avoid unnecessarily reassigning the same methods (in case there are multiple instances of Transcrypt running).

Fixes TranscryptOrg#816
Assigning functions directly to the prototypes of Number, Array, String, etc., causes them to be enumerable, meaning they can show up in "for .. in .." loops and cause unexpected behavior!

Instead we can use `Object.defineProperty` (which we already have a helper for called `__setproperty__`. With this method, i) new props are non-enumerable by default, and ii) we can avoid unnecessarily reassigning the same methods, in case there are multiple instances of Transcrypt running.
I noted that `list` methods get assigned to `Array.prototype`, `str` methods get assigned to `String.prototype`, etc.
`dict` methods do not get assigned to `Object.prototype` (I am guessing this is for a good reason)

The current code has the `dict` methods being to each new instance in the constructor. It is tidier to declare them on the `dict` prototype and then create each dict instance from that.
This is also more efficient because properties placed on the dict prototype need only be declared once, as opposed to being declared for each dict.
@JGreenlee
Copy link
Author

JGreenlee commented Aug 23, 2024

Put dict methods on the dict.prototype

I noticed that list methods get assigned to Array.prototype, str methods get assigned to String.prototype, etc.
dict methods do not get assigned to Object.prototype (I am guessing this is for a good reason)

The current code has the dict methods being added to each new instance in the constructor; but it is tidier to declare them on the dict prototype and then create each dict instance from that.
This is also more efficient because properties placed on the dict prototype need only be declared once, as opposed to being declared for each dict.

@JGreenlee
Copy link
Author

The properties & prototype chain for an example dict foo:

Before (on master):

image

After changes

image

@JennaSys
Copy link
Collaborator

This is one I've had an eye on to tackle at some point sooner than later, so thanks for looking into it. I can't dig into it right at this moment, but are there any possible unintended side effects by changing this that you can think of?

@JGreenlee
Copy link
Author

JGreenlee commented Aug 24, 2024

If for some reason, someone was expecting the methods of str or list to be included while iterating over an Array or String, they would no longer see those.

for (let e in []) {
  console.log(e)
}
// on master, prints all the methods of py `list`
// after this PR, prints nothing

I don't know why someone would be relying on that behavior though.

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.

Prototype pollution
2 participants