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

add option to disable devtools #7096

Merged
merged 8 commits into from Sep 13, 2016

Conversation

Projects
None yet
7 participants
@minggo
Contributor

minggo commented Sep 5, 2016

Sometimes you want to create a browser window that can not open devtools for security. This PR just do it, you create a browser window like

cont window = new BrowserWindow({width: 800, height: 600, disableDevTools: true})
@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 5, 2016

To be more consistent with our existing API for creating a BrowserWindow. This should probably be a devTools property on the webPreferences object with a default of true.

Manually setting to false would disable devtools.

This is consistent with properties like the nodeIntegration property.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 5, 2016

@MarshallOfSound thanks for the reply. But i think it is different, with this PR, you can not enable DevTools again.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 5, 2016

@minggo I am aware of that. That is the same experience that you get with all the properties in webPreferences. You can set them once on the original call to new BrowserWindow and can't change them once the BrowserWindow has been created.

I wasn't suggesting a method rather a property on this object. Just a semantic movement of where the config property should go

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 5, 2016

@MarshallOfSound got it. So where do you think is the proper module to add it?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 5, 2016

Just IMO, but just moving it to the webPreferences option makes more sense than where it is at the moment.

https://github.com/minggo/electron/blob/0d7e7be7481ee0fe077257d48e130b768789a145/atom/browser/api/atom_api_window.cc#L75

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 5, 2016

Yep, it is better. I will modify it. Thanks.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 5, 2016

@MarshallOfSound updated. thanks

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 6, 2016

The ci failed, the error message is

Done processing atom/browser/api/atom_api_power_monitor.cc
Total errors found: 1
Traceback (most recent call last):
  File "./script/cpplint.py", line 45, in <module>
    sys.exit(main())
  File "./script/cpplint.py", line 26, in main
    call_cpplint(list(set(files) - set(IGNORE_FILES)))
  File "./script/cpplint.py", line 41, in call_cpplint
    execute([sys.executable, cpplint] + files)
  File "/home/travis/build/electron/electron/script/lib/util.py", line 170, in execute
    raise e

I don't modify atom_api_power_monitor.cc. @MarshallOfSound could you please take a look? Thanks.

@@ -83,7 +83,7 @@ Window::Window(v8::Isolate* isolate, v8::Local<v8::Object> wrapper,
v8::Local<v8::Value> transparent;
if (options.Get("transparent", &transparent))
web_preferences.Set("transparent", transparent);

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 6, 2016

Member

The error on your build is coming from a linting issue with this line

atom/browser/api/atom_api_window.cc:86: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]

You need to get rid of this whitespace 👍

This comment has been minimized.

@minggo

minggo Sep 6, 2016

Contributor

Thanks. But the error message is strange. And i think add the whitespace is more readable. Any way, i removed the whitespace.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 6, 2016

Member

You can (and probably should) have the line break.

But you had some stray " " characters on that line. And it's against the style guide to have space characters at the end of a line. Let alone on an empty line 👍

This comment has been minimized.

@minggo

minggo Sep 6, 2016

Contributor

Yep, you are right. And it seems the travis ia32 job broken.

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Sep 6, 2016

Should also add a check in WebContents::InspectElement and WebContents::InspectServiceWorker, to avoid unnecessary devtools agent creation.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 6, 2016

@deepak1556 thanks for you reply. I am not familiar with WebContents::InspectElement and WebContents::InspectServiceWorker, could you tell me how to use it to open devtools? I found i can not open devtools when do like

mainWindow = new BrowserWindow({width: 800, height: 600, webPreferences: { disableDevTools: true } })
mainWindow.webContents.inspectElement(400, 300)
mainWindow.webContents.inspectServiceWorker()
@deepak1556

This comment has been minimized.

Member

deepak1556 commented Sep 6, 2016

@minggo you are right the view will not be created, but i was referring to short circuiting those methods since devtools is disabled.

Edit: should add the option to docs.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 6, 2016

@deepak1556 yep, i found WebContents::InspectElement and WebContents::InspectServiceWorker invoked OpenDevTools and create an agent. Did you mean i should add check in these two functions like this:

void WebContents::InspectElement(int x, int y) {
  if (type_ == REMOTE)
    return;

  if (disable_devtools_)
    return;

  if (!managed_web_contents()->GetDevToolsWebContents())
    OpenDevTools(nullptr);
  scoped_refptr<content::DevToolsAgentHost> agent(
    content::DevToolsAgentHost::GetOrCreateFor(web_contents()));
  agent->InspectElement(x, y);
}

I will modify the doc.

minggo added some commits Sep 6, 2016

WebContents::InspectElement and WebContents::InspenctServiceWorker re…
…turn immediately if DevTools is disabled
@minggo

This comment has been minimized.

Contributor

minggo commented Sep 6, 2016

@deepak1556 done, please take a look. Thanks.

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Sep 6, 2016

👍

@@ -323,6 +323,9 @@ class WebContents : public mate::TrackableObject<WebContents>,
// Whether background throttling is disabled.
bool background_throttling_;
// // Whether to disable devtools.

This comment has been minimized.

@kevinsawicki

kevinsawicki Sep 6, 2016

Contributor

Duplicate // here, only need one set of //.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Sep 6, 2016

This should probably be a devTools property on the webPreferences object with a default of true.

I agree with @MarshallOfSound, I think devTools defaulting to true is the right pattern to follow here to mirror the other options, like nodeIntegration.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 7, 2016

@kevinsawicki but the property is to disable DevTools, if the default value is true, then it will break compatibility.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Sep 7, 2016

but the property is to disable DevTools, if the default value is true, then it will break compatibility.

Yeah, I think the property should be to enable dev tools, default to true and setting it to false disables it, the same as nodeIntegration. That shouldn't break compatibility

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 7, 2016

But now we can open DevTools by BrowserWindow.webPreference.openDevTools(), which mean the default behavior is what we want. So i think add a property that disableDevTools is more reasonable.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 7, 2016

@minggo What @kevinsawicki and I are saying is that the syntax should be as follows

// Enables devTools - Notice no extra options (it defaults to true)
new BrowserWindow({
  width: 400,
  height: 400,
});

// Disables devTools - Specifically disable the option
new BrowserWindow({
  width: 400,
  height: 400,
  webPreferences: {
    devTools: false
  }
})
@minggo

This comment has been minimized.

Contributor

minggo commented Sep 7, 2016

@MarshallOfSound both are ok too me. I will modify it to match other options.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 7, 2016

@MarshallOfSound @kevinsawicki done, please take a look. Thanks.

@fritx

This comment has been minimized.

Contributor

fritx commented Sep 7, 2016

Is there any workaround for this -- to disable devtools?

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 7, 2016

@fritx sorry, what do you mean? Did you mean if there is other way to disable devtools? If so, i can not find a way that can disable devtools that can not be enabled again.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 7, 2016

Is there any workaround for this -- to disable devtools?

If you mean once you set this option, is there a way for the user to re-enable the devtools. The answer is yes, there is always a way. This doesn't offer complete prevention of devtools usage it just makes it a lot harder for an "average" user to open devtools.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 7, 2016

@MarshallOfSound good to know. Would you mind to show me how to re-enable the devtools? I am interested with it, and want to completely prevent it in my app. Thanks.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 7, 2016

@minggo It would be as simple as editing the JS code you distribute with your app and changing this option. Admittedly obfuscation could make this quite tricky, but certainly possible.

If you want to completely prevent it then you would have to ship a custom version of Electron with the flag you just created in this PR forced to false always. Then the editable JS code can't alter the behavior.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 7, 2016

@MarshallOfSound got it. Indeed i have a custom version of Electron and custom version of ASAR, so the scripts are encrypted.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 7, 2016

Indeed i have a custom version of Electron and custom version of ASAR, so the scripts are encrypted.

This sounds interesting. Is this custom version available anywhere? That kind of source protection would interest a lot of people

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 7, 2016

No, currently it is in a private repo for the company's benefit. I have a custom ASAR to encrypted scripts and decrypt them in custom Electron.

I can not disable readfile and readfileSync because internal scripts also use it to load scripts, so i disabeld readdir and readdirSync, then developers don't know its folder structure.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 7, 2016

so i disabeld readdir and readdirSync, then developers don't know its folder structure.

I mean these two functions are blocked if you want to read specific asar files.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 9, 2016

@MarshallOfSound @deepak1556 Is there any problem of this PR?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 9, 2016

@minggo It LGTM, just needs someone from the core team to look at it and merge

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Sep 9, 2016

In Electron the devtools feature is provided by the menu, so it you don't have a "Open DevTools" menu there is no normal way to open devtools, adding a devtools option doesn't make any difference. Or is there something I'm missing?

If you want to have a way to disable devtools completely, the proper way is to add a variable in electron.gyp and uses #defines to disable the devtools code.

Apart from the openDevTools method, you also need to disable the --remote-debugg-port switch to protect your code, which is in brightray.

@minggo

This comment has been minimized.

Contributor

minggo commented Sep 12, 2016

@zcbenz adding this option is because we just project some browser window. For example, i have a tool supports plugins, every plugin is a browser window, but some plugin needs to be protected, which means don't allow to open devtools.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Sep 12, 2016

@minggo It can be done by using an if condition:

{
  label: 'Open DevTools',
  click (win) {
    if (isPluginWindow(win))
      dialog.showMessageBox('You can not open devtools for this window')
    else
      win.openDevtools()
  }
}
@jwu

This comment has been minimized.

jwu commented Sep 12, 2016

@zcbenz I think the problem for this approach is that user still can open the dev-tools via the script or the other windows' dev-tools.

In our case, user can have many windows opened. And we have a window-list in main-process. User can use Main Window's dev-tools to access this window-list and finally get the webContents of the target window, then they can open the dev-tools by calling the API relative with it (webContents.openDevtools()).

@minggo 's method prevent people doing these kinds of things and protect some specific window being hacked by other developer.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Sep 13, 2016

@jwu Thanks for the clarification, it sounds quite reasonable. 👍

@zcbenz zcbenz merged commit 4eba809 into electron:master Sep 13, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment