8000 fix: remove bad usages of for-in and guard against it by MarshallOfSound · Pull Request #22616 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: remove bad usages of for-in and guard against it #22616

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 17, 2020
Merged

fix: remove bad usages of for-in and guard against it #22616

merged 4 commits into from
Mar 17, 2020

Conversation

MarshallOfSound
Copy link
Member

for (... in ...) is a bad idea as it will pick up everything on the prototype as well not just keys on that object. Where appropriate I've replaced it with for (const key of Object.keys...) and enabled the eslint rule that guards against it so that future occurrences to not slip through.

Fixes #22615

Notes: fixed issue where mutating the global Object prototype could cause internal Electron logic to throw errors

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Mar 9, 2020
nornagon
nornagon approved these changes Mar 9, 2020
View reviewed changes
Copy link
Contributor
@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

fantastic, didn't occur to me that there would be a lint rule for this. nice

samuelmaddock
samuelmaddock reviewed Mar 9, 2020
View reviewed changes
Copy link
Member
@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

Made some suggestions to make it more clear about the check for non-inherited properties.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Mar 10, 2020
nornagon
nornagon suggested changes Mar 11, 2020
View reviewed changes
@ckerr
Copy link
Member
ckerr commented Mar 12, 2020 •
edited
Loading

I'm ✔️ on putting this in new releases, but before we backport this -- is this fixing some known existing problems?

I think some of these objects being looped through are coming from users, e.g. extraHeaders is coming from the 'headers' property of the options object passed to ClientRequest's constructor. It's possible that some oddball app out in the wild works under for...in but not under for..of Object.keys().

Which is why I ask if this is fixing any known bugs. If this is a cleanliness PR, I don't see the upside in backporting.

@ckerr
Copy link
Member
ckerr commented Mar 12, 2020

...or I could've just, you know, read the description and click through to #22615 to see what this fixes.

Perhaps a more limited scope of changes in the backports?

@MarshallOfSound
Copy link
Member Author

@ckerr Sending properties of the headers object prototype through would also be considered a bug (hence the change instead of the lint-ignore). These cases were carefully chosen as to whether to update the usage or ignore the lint rule. Things like remote were left alone out of fear of side-effect breakages and will be dealt with later, the headers object though it is never intended or expected behavior to send a stringified prototype function as a header so I switched it to not do that any more.

MarshallOfSound and others added 4 commits March 16, 2020 12:22
@MarshallOfSound
fix: remove bad usages of for-in and guard against it
0f904c3
@MarshallOfSound @samuelmaddock
Apply suggestions from code review
0d73581 
Co-Authored-By: Samuel Maddock <samuel.maddock@gmail.com>
@MarshallOfSound @nornagon
Apply suggestions from code review
8cc86b9 
Co-Authored-By: Jeremy Apthorp <jeremya@chromium.org>
@MarshallOfSound
Update remote.js
f9a123d
@MarshallOfSound MarshallOfSound merged commit 5e4e50c into master Mar 17, 2020
@release-clerk
Copy link
release-clerk bot commented Mar 17, 2020

Release Notes Persisted

fixed issue where mutating the global Object prototype could cause internal Electron logic to throw errors

@MarshallOfSound MarshallOfSound deleted the bad-for-in branch March 17, 2020 20:17
This was referenced Mar 17, 2020
Merged
Merged
@trop
Copy link
Contributor
trop bot commented Mar 17, 2020

I have automatically backported this PR to "9-x-y", please check out #22727

@trop trop bot removed the target/9-x-y label Mar 17, 2020
@trop trop bot mentioned this pull request Mar 17, 2020
Merged
@trop trop bot added the in-flight/9-x-y label Mar 17, 2020
@trop
Copy link
Contributor
trop bot commented Mar 17, 2020

I have automatically backported this PR to "8-x-y", please check out #22728

@trop
Copy link
Contributor
trop bot commented Mar 17, 2020

I have automatically backported this PR to "7-1-x", please check out #22729

@trop trop bot removed the target/8-x-y label Mar 17, 2020
sentialx pushed a commit to sentialx/electron that referenced this pull request Apr 7, 2020
@MarshallOfSound @samuelmaddock
@nornagon @sentialx
fix: remove bad usages of for-in and guard against it (electron#22616)
19f5d4e 
* fix: remove bad usages of for-in and guard against it

* Apply suggestions from code review

Co-Authored-By: Samuel Maddock <samuel.maddock@gmail.com>

* Apply suggestions from code review

Co-Authored-By: Jeremy Apthorp <jeremya@chromium.org>

* Update remote.js

Co-authored-by: Samuel Maddock <samuel.maddock@gmail.com>
Co-authored-by: Jeremy Apthorp <jeremya@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelmaddock samuelmaddock samuelmaddock left review comments

@ckerr ckerr ckerr approved these changes

@nornagon nornagon nornagon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding any Object prototype throws an error
4 participants
@MarshallOfSound @ckerr @nornagon @samuelmaddock
0