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

feat: sandbox preloads by default #32869

Merged
merged 24 commits into from Jul 11, 2022
Merged

feat: sandbox preloads by default #32869

merged 24 commits into from Jul 11, 2022

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Feb 11, 2022 •
edited

BREAKING CHANGE

Description of Change

Ref #28466 (comment), #32868

NB. breaking change is already documented in breaking-changes.md: https://github.com/electron/electron/blob/main/docs/breaking-changes.md#default-changed-renderers-without-nodeintegration-true-are-sandboxed-by-default

Checklist

Release Notes

Notes: Renderers are now sandboxed by default unless nodeIntegration: true or sandbox: false is specified.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 11, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 18, 2022
@nornagon nornagon added semver/major incompatible API changes no-backport labels Mar 28, 2022
@nornagon nornagon removed the wip ⚒ label Mar 30, 2022
jkleinsc
jkleinsc approved these changes Jun 8, 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

zcbenz
zcbenz reviewed Jun 9, 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 looks good to me.

The "BrowserWindow module "webPreferences" option child windows works in a scriptable popup" test is failing.

Also do we have a test verifying preload is sandboxed by default?

@nornagon
Copy link
Member Author

nornagon commented Jun 9, 2022

Hm... that test is failing because it uses nodeIntegrationInSubframes: true without nodeIntegration: true, so the test gets sandboxed, which somehow seems to break the preload script? Even though the preload script just exposes ipcRenderer.

I'm not quite sure what's going on here, it seems to me like that test ought to work unmodified.

miniak
miniak reviewed Jun 14, 2022
View reviewed changes
spec/webview-spec.js Outdated Show resolved Hide resolved
@nornagon nornagon requested a review from a team as a code owner June 14, 2022 23:35
itsananderson
itsananderson approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@itsananderson itsananderson 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

@nornagon
Copy link
Member Author

Mac failures are flake; merging.

@nornagon nornagon merged commit 4190ec2 into main Jul 11, 2022
@nornagon nornagon deleted the sandbox-by-default branch July 11, 2022 23:28
@release-clerk
Copy link

release-clerk bot commented Jul 11, 2022

Release Notes Persisted

Renderers are now sandboxed by default unless nodeIntegration: true or sandbox: false is specified.

VerteDinde pushed a commit that referenced this pull request Jul 29, 2022
@nornagon @VerteDinde
feat: sandbox preloads by default (#32869)
bc0e154
@VerteDinde VerteDinde mentioned this pull request Jul 29, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Jul 29, 2022

@VerteDinde has manually backported this PR to "20-x-y", please check out #35125

VerteDinde added a commit that referenced this pull request Jul 29, 2022
@VerteDinde @nornagon
feat: sandbox preloads by default (#35125)
08aa578 
feat: sandbox preloads by default (#32869)

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
@commit-lint commit-lint bot mentioned this pull request Sep 15, 2022
Open
@Madff386 Madff386 mentioned this pull request Sep 18, 2022
Merged
schetle pushed a commit to schetle/electron that referenced this pull request Nov 3, 2022
@nornagon @schetle
feat: sandbox preloads by default (electron#32869)
05406e7
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@nornagon @khalwa
feat: sandbox preloads by default (electron#32869)
72fc624
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz left review comments

@itsananderson itsananderson itsananderson approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

@miniak miniak miniak approved these changes

Assignees
No one assigned
Labels
api-review/approved ✅ no-backport semver/major incompatible API changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nornagon @itsananderson @jkleinsc @zcbenz @miniak