doyensec / electronegativity

Electronegativity is a tool to identify misconfigurations and security anti-patterns in Electron applications.
Apache License 2.0
958 stars 65 forks source link

Support the setWindowOpenHandler API in Electron 12+ #92

Closed mitchchn closed 1 year ago

mitchchn commented 3 years ago

Is your feature request related to a problem? Please describe.

Electron 12 has a new API to capture and prevent window creation, webContents.setWindowOpenHandler. See: https://www.electronjs.org/docs/tutorial/security#how-10.

The existing new-window event has been deprecated.

Describe the solution you'd like

LIMIT_NAVIGATION_JS_CHECK should detect the presence of the new handler API and treat it the same way as the new-window event, assuming it is an appropriate alternative (or addition).

Describe alternatives you've considered

It's possible to continue using the deprecated event. It is not entirely clear to me how these APIs interact when used together, or whether they are completely interchangeable .

phosphore commented 3 years ago

Hello Mitch! I added to the LIMIT_NAVIGATION_JS_CHECK check the support for setWindowOpenHandler. The change is live from v1.9.1. Let me know if it works for you.

About your question, the webContents.on("new-window") et al works fine in v12 and previous electron version, for the moment it's just marked as deprecated. Note that setWindowOpenHandler was sporadically working on v12 (https://github.com/electron/electron/issues/27967):

As a temporary fix before #28498, for anyone looking to prevent navigations to untrusted origins it should still possible to use the web-contents-created event emitted when a new webContents is created and attach then your new-window, will-navigate, and setWindowOpenHandler handlers:


app.on('web-contents-created', (createEvent, contents) => {

contents.on('new-window', newEvent => { console.log("Blocked by 'new-window'") newEvent.preventDefault(); });

contents.on('will-navigate', newEvent => { console.log("Blocked by 'will-navigate'") newEvent.preventDefault() });

contents.setWindowOpenHandler(({ url }) => { if (url.startsWith("https://doyensec.com/")) { setImmediate(() => { shell.openExternal(url); }); return { action: 'allow' } } else { console.log("Blocked by 'setWindowOpenHandler'") return { action: 'deny' } } })

});



>[Link to the Fiddle gist used for testing](https://gist.github.com/phosphore/311545f56826d67a55aa2c3e6d17dcf9). This works from 12.0.0 to the latest 12.0.2 as for now.

_Originally posted by @phosphore in https://github.com/electron/electron/issues/27967#issuecomment-812840093_
mscharley commented 2 years ago

I'm still getting an error flagged in 1.9.1 from LIMIT_NAVIGATION_GLOBAL_CHECK when using setWindowOpenHandler as below:

    this._window.webContents.setWindowOpenHandler((details) => {
      if (this.isAllowedInternalNavigationUrl(details.url)) {
        return { action: 'allow' };
      } else {
        this.tryExternalNavigation(details.url);
        return { action: 'deny' };
      }
    });
phosphore commented 1 year ago

This should now be resolved with v1.10. The issue was solved in ElectroNG since its early releases, but now I've ported it to electronegativity.

goosewobbler commented 1 year ago

@phosphore This still seems to produce a warning (see the above PR), am I doing something wrong? Our usage of setWindowOpenHandler: https://github.com/floating/frame/blob/develop/main/windows/window.ts#L41 https://github.com/floating/frame/blob/develop/main/windows/window.ts#L68 https://github.com/floating/frame/blob/develop/main/windows/index.ts#L383

phosphore commented 1 year ago

No, it LGTM. Could you try again with the latest d0d4445? Thanks for catching that.

goosewobbler commented 1 year ago

Thanks for the quick response, looks like it's fixed!