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

Remove for-in loop on array #3

Merged
merged 1 commit into from Nov 24, 2021
Merged

Conversation

MarcPorciuncula
Copy link

@MarcPorciuncula MarcPorciuncula commented Oct 5, 2021

Hey there, I'm trying to load amplitude-js into a browser context that also has some third party scripts that we can't control. One such script is modifying the Array prototype and adding additional enumerable properties.

These additional properties are picked up by the for-in loop in enumerize and result in the evaluation of toUpperCase on potentially non-string values, causing a crash.

Replacing the for-in loop with a counter based for loop would make this safer.

@GalvinGao
Copy link

GalvinGao commented Oct 22, 2021

Please merge this PR as this problem has already causing some of our user using a lower version of Android Webview with our App to crash due to an issue described in amplitude/Amplitude-JavaScript#435. In-house solution was to create a patch using patch-package. See the issue for potential fixes when this PR is not yet being merged.

@chrskrchr
Copy link

@qingzhuozhen - could you please review this PR? or refresh the fork from upstream? This bug has been addressed in the upstream ua-parser-js lib:

faisalman#547

@seanparmelee
Copy link

Hey @jooohhn, any chance you can help move this one along?

Copy link

@jooohhn jooohhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

@jooohhn jooohhn merged commit 59cc613 into amplitude:master Nov 24, 2021
@chrskrchr
Copy link

chrskrchr commented Nov 29, 2021

@jooohhn - any ETA on when we could expect for this to be pulled into the Amplitude-JavaScript project to address faisalman#435?

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