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.

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

fix: disable nodeIntegrationInWorker for certain Worker types #35919

Merged
merged 1 commit into from Oct 12, 2022
Merged

fix: disable nodeIntegrationInWorker for certain Worker types #35919

merged 1 commit into from Oct 12, 2022

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Oct 5, 2022 •
edited

Description of Change

Closes #31452.

Out-of-process Workers present significant crash and bug surface area as a result of Electron's current inability to control their sandbox policies. As a result, we shouldn't initialize Node environment for SharedWorkers and ServiceWorkers to avoid problems when in sandboxed environment.

After this change, the nodeIntegrationInWorker attribute should only affect workers that run in-process of the associated BrowserWindow.

Checklist

Release Notes

Notes: Removed support for nodeIntegrationInWorker in Service Workers and Shared Workers owing to sandboxing policies.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/21-x-y PR should also be added to the "21-x-y" branch. target/22-x-y PR should also be added to the "22-x-y" branch. labels Oct 5, 2022
@codebytere codebytere changed the title fix: disable nodeIntegrationInWorker for certain Worker types fix: disable nodeIntegrationInWorker for certain Worker types Oct 5, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 5, 2022
@codebytere codebytere marked this pull request as draft October 5, 2022 15:08
@codebytere codebytere force-pushed the disable-worker-node-for-some-workers branch 2 times, most recently from df52c30 to 30aa450 Compare October 5, 2022 17:44
@codebytere codebytere marked this pull request as ready for review October 5, 2022 17:44
deepak1556
deepak1556 approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Given that we supported this feature for these worker types before, should this be a semver/minor ? Also can you add release notes since it is user facing change. Thanks!

@codebytere codebytere added semver/minor backwards-compatible functionality and removed semver/patch backwards-compatible bug fixes labels Oct 6, 2022
zcbenz
zcbenz approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Can you also mention this behavior in nodeIntegrationInWorker's docs?

@codebytere codebytere force-pushed the disable-worker-node-for-some-workers branch from c39966f to d7fa48c Compare October 6, 2022 12:27
@codebytere
Copy link
Member Author

@zcbenz / @deepak1556 would you mind adding API LGTM?

zcbenz
zcbenz approved these changes Oct 12, 2022
View reviewed changes
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

API LGTM

jkleinsc
jkleinsc approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

API LGTM

@jkleinsc jkleinsc removed the new-pr 🌱 PR opened in the last 24 hours label Oct 12, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 12, 2022
@jkleinsc jkleinsc removed new-pr 🌱 PR opened in the last 24 hours api-review/requested 🗳 labels Oct 12, 2022
@electron-cation electron-cation bot added api-review/requested 🗳 new-pr 🌱 PR opened in the last 24 hours labels Oct 12, 2022
@jkleinsc jkleinsc removed the new-pr 🌱 PR opened in the last 24 hours label Oct 12, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 12, 2022
@jkleinsc jkleinsc merged commit 7ce94eb into main Oct 12, 2022
@jkleinsc jkleinsc deleted the disable-worker-node-for-some-workers branch October 12, 2022 14:36
@release-clerk
Copy link

release-clerk bot commented Oct 12, 2022

Release Notes Persisted

Removed support for nodeIntegrationInWorker in Service Workers and Shared Workers owing to sandboxing policies.

This was referenced Oct 12, 2022
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Oct 12, 2022

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

@trop trop bot added in-flight/21-x-y and removed target/21-x-y PR should also be added to the "21-x-y" branch. labels Oct 12, 2022
@trop
Copy link
Contributor

trop bot commented Oct 12, 2022

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

@trop trop bot added in-flight/22-x-y merged/22-x-y PR was merged to the "22-x-y" branch. and removed target/22-x-y PR should also be added to the "22-x-y" branch. labels Oct 12, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 13, 2022
@trop trop bot added merged/21-x-y PR was merged to the "21-x-y" branch. and removed in-flight/22-x-y labels Oct 13, 2022
@commit-lint commit-lint bot mentioned this pull request Feb 8, 2023
Open
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@codebytere @khalwa
fix: disable nodeIntegrationInWorker for certain Worker types (elec…
c3342cf 
…tron#35919)

fix: disable nodeIntegrationInWorker for certain Worker types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

Assignees
No one assigned
Labels
api-review/requested 🗳 merged/21-x-y PR was merged to the "21-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Crash on attempt to initialize Node Environment with enabled nodeIntegrationInWorker.
4 participants
@codebytere @jkleinsc @zcbenz @deepak1556