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

Support the setWindowOpenHandler API in Electron 12+ #92

Closed
mitchchn opened this issue May 3, 2021 · 6 comments
Closed

Support the setWindowOpenHandler API in Electron 12+ #92

mitchchn opened this issue May 3, 2021 · 6 comments

Comments

@mitchchn
Copy link

mitchchn commented May 3, 2021

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
Copy link
Contributor

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 (electron/electron#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. This works from 12.0.0 to the latest 12.0.2 as for now.

Originally posted by @phosphore in electron/electron#27967 (comment)

@mscharley
Copy link

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 added a commit that referenced this issue Dec 7, 2022
@phosphore
Copy link
Contributor

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
Copy link

goosewobbler commented Dec 7, 2022

phosphore added a commit that referenced this issue Dec 8, 2022
@phosphore
Copy link
Contributor

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

@goosewobbler
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants