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

feat: sandbox renderer processes for cross-origin frames #18650

Merged
merged 1 commit into from Jun 20, 2019

Conversation

@miniak
Copy link
Contributor

miniak commented Jun 5, 2019

Description of Change

Cross-origin frames, which have a separate renderer process due to site isolation do not have their own webContents and therefore use the webPreferences of the main frame. When nodeIntegrationInSubFrames is not enabled, there is no reason not to sandbox the renderer process as we are not executing any internal JavaScript code or initializing node, which would be impacted by the sandbox.

Follow up to #15821

Checklist

Release Notes

Notes: Renderer processes hosting cross-origin frames are now sandboxed unless the parent BrowserWindow enables nodeIntegrationInSubFrames.

@miniak miniak added the wip label Jun 5, 2019
@miniak miniak force-pushed the miniak/sandbox-cross-origin-iframes branch from e610c87 to a148167 Jun 5, 2019
@miniak miniak self-assigned this Jun 5, 2019
@miniak miniak force-pushed the miniak/sandbox-cross-origin-iframes branch 3 times, most recently from 4341891 to d36007a Jun 5, 2019
Copy link
Member

MarshallOfSound left a comment

Why? What's the potential impact? Why do we need to explicitly do this?

@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Jun 6, 2019

@MarshallOfSound I've added a bit more text to the description

Why?

improved security, cross-origin iframes are sandboxed even if the main page is not

What's the potential impact?

there should be no negative effects

Why do we need to explicitly do this?

because there is no way to explicitly enable sandbox on iframes

@miniak miniak requested a review from MarshallOfSound Jun 6, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 6, 2019
@miniak miniak marked this pull request as ready for review Jun 7, 2019
@miniak miniak requested review from deepak1556, nornagon, zcbenz and ppontes Jun 7, 2019
atom/browser/atom_browser_client.cc Outdated Show resolved Hide resolved
atom/browser/web_contents_preferences.cc Outdated Show resolved Hide resolved
atom/browser/web_contents_preferences.cc Outdated Show resolved Hide resolved
atom/browser/web_contents_preferences.cc Outdated Show resolved Hide resolved
@miniak miniak force-pushed the miniak/sandbox-cross-origin-iframes branch 4 times, most recently from ae37cee to 5b32443 Jun 13, 2019
@miniak miniak changed the base branch from master to miniak/app-metrics Jun 13, 2019
@miniak miniak requested a review from MarshallOfSound Jun 13, 2019
@zcbenz
zcbenz approved these changes Jun 14, 2019
@miniak miniak force-pushed the miniak/sandbox-cross-origin-iframes branch from 5b32443 to e46e665 Jun 14, 2019
@miniak miniak changed the base branch from miniak/app-metrics to master Jun 14, 2019
@miniak miniak force-pushed the miniak/sandbox-cross-origin-iframes branch 2 times, most recently from 56c1b56 to b29467f Jun 15, 2019
@miniak miniak force-pushed the miniak/sandbox-cross-origin-iframes branch 2 times, most recently from cd5e900 to 17888ca Jun 15, 2019
@miniak miniak removed the wip label Jun 15, 2019
@miniak miniak mentioned this pull request Jun 15, 2019
0 of 6 tasks complete
@miniak miniak force-pushed the miniak/sandbox-cross-origin-iframes branch from 17888ca to cbc2f21 Jun 15, 2019
@miniak

This comment has been minimized.

Copy link
Contributor Author

miniak commented Jun 17, 2019

@MarshallOfSound can you please check again?

@miniak miniak force-pushed the miniak/sandbox-cross-origin-iframes branch from cbc2f21 to 28017d1 Jun 20, 2019
@alexeykuzmin alexeykuzmin merged commit f3f2990 into master Jun 20, 2019
13 checks passed
13 checks passed
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190620.13 succeeded
Details
electron-arm64-testing Build #20190620.13 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jun 20, 2019

Release Notes Persisted

Renderer processes hosting cross-origin frames are now sandboxed unless the parent BrowserWindow enables nodeIntegrationInSubFrames.

@alexeykuzmin alexeykuzmin deleted the miniak/sandbox-cross-origin-iframes branch Jun 20, 2019
@miniak miniak mentioned this pull request Jun 23, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.