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

fix contextIsolation issue while webPreference sandbox is on #10214

Merged
merged 4 commits into from Aug 29, 2017

Conversation

Projects
None yet
5 participants
@psh0628

psh0628 commented Aug 7, 2017

Fixes #9971

@jkleinsc jkleinsc requested a review from Aug 8, 2017

@tarruda

I think this is a good opportunity to expose webFrame to sandboxed renderer.

const {ipcRenderer, webFrame} = require('electron')
window.foo = 3
webFrame.executeJavaScript('window.preloadExecuteJavaScriptProperty = 1234;')

This comment has been minimized.

@tarruda

tarruda Aug 8, 2017

Contributor

Instead of removing this, why not add webFrame to sandboxed renderer? A simple addition to https://github.com/electron/electron/blob/master/lib/sandboxed_renderer/api/exports/electron.js should do the trick.

This comment has been minimized.

@psh0628

psh0628 Aug 8, 2017

yes, I added webFrame to sandboxed renderer. thanks

v8::Local<v8::Context> RendererClientBase::GetContext(
blink::WebFrame* frame, v8::Isolate* isolate) {
if (isolated_world())
return frame->worldScriptContext(isolate, World::ISOLATED_WORLD);

This comment has been minimized.

@kevinsawicki

kevinsawicki Aug 8, 2017

Contributor

👕 Please use spaces instead of hard tabs for indentation

This comment has been minimized.

@psh0628

psh0628 Aug 8, 2017

will fix this

@@ -166,7 +166,7 @@ Currently the `require` function provided in the preload scope exposes the
following modules:
- `child_process`
- `electron` (crashReporter, remote and ipcRenderer)
- `electron` (crashReporter, remote, ipcRenderer, and webFrame)

This comment has been minimized.

@kevinsawicki

kevinsawicki Aug 8, 2017

Contributor

Maybe this is a good time to add the supported modules as a sub-list now that it is starting to get long.

- `electron`
  - `crashReporter`
  - `ipcRenderer`
  - `remote`
  - `webFrame`

Which will render like:

  • electron
    • crashReporter
    • ipcRenderer
    • remote
    • webFrame

This comment has been minimized.

@psh0628

psh0628 Aug 9, 2017

will fix this

@psh0628

This comment has been minimized.

psh0628 commented Aug 23, 2017

@tarruda @kevinsawicki
I took care of your comments and I am still waiting for your approval.

sungpark added some commits Jul 28, 2017

sungpark
fix contextIsolation issue while webPreference sandbox is on
contextIsolation didn't work while sandbox is on. The fix is contextIsolation picked up while sandbox on
sungpark
@miniak

miniak requested changes Aug 23, 2017 edited

why is your PR changing 166 files? did you merge something instead of rebasing your changes on top of latest master? please use git properly. it's not possible to review the actual change like this.

sungpark
@zeke

This comment has been minimized.

Member

zeke commented Aug 28, 2017

Hey @tarruda and @miniak can you take another look?

@zeke zeke merged commit c691896 into electron:master Aug 29, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@welcome

This comment has been minimized.

welcome bot commented Aug 29, 2017

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment